Re: [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

Hi Matthias,

On Tue, 2016-06-07 at 18:59 +0200, Matthias Brugger wrote:
> 
> On 03/06/16 15:11, Matthias Brugger wrote:
> >
> >
> [...]
> 
> >>>>>>>>>> +
> >>>>>>>>>> +            smp_mb(); /* modify jump before enable thread */
> >>>>>>>>>> +        }
> >>>>>>>>>> +
> >>>>>>>>>> +        cmdq_thread_writel(thread, task->pa_base +
> >>>>>>>>>> task->command_size,
> >>>>>>>>>> +                   CMDQ_THR_END_ADDR);
> >>>>>>>>>> +        cmdq_thread_resume(thread);
> >>>>>>>>>> +    }
> >>>>>>>>>> +    list_move_tail(&task->list_entry, &thread->task_busy_list);
> >>>>>>>>>> +    spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static void cmdq_handle_error_done(struct cmdq *cmdq,
> >>>>>>>>>> +                   struct cmdq_thread *thread, u32 irq_flag)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_task *task, *tmp, *curr_task = NULL;
> >>>>>>>>>> +    u32 curr_pa;
> >>>>>>>>>> +    struct cmdq_cb_data cmdq_cb_data;
> >>>>>>>>>> +    bool err;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (irq_flag & CMDQ_THR_IRQ_ERROR)
> >>>>>>>>>> +        err = true;
> >>>>>>>>>> +    else if (irq_flag & CMDQ_THR_IRQ_DONE)
> >>>>>>>>>> +        err = false;
> >>>>>>>>>> +    else
> >>>>>>>>>> +        return;
> >>>>>>>>>> +
> >>>>>>>>>> +    curr_pa = cmdq_thread_readl(thread, CMDQ_THR_CURR_ADDR);
> >>>>>>>>>> +
> >>>>>>>>>> +    list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
> >>>>>>>>>> +                 list_entry) {
> >>>>>>>>>> +        if (curr_pa >= task->pa_base &&
> >>>>>>>>>> +            curr_pa < (task->pa_base + task->command_size))
> >>>>>>>>>
> >>>>>>>>> What are you checking here? It seems as if you make some implcit
> >>>>>>>>> assumptions about pa_base and the order of execution of
> >>>>>>>>> commands in the
> >>>>>>>>> thread. Is it save to do so? Does dma_alloc_coherent give any
> >>>>>>>>> guarantees
> >>>>>>>>> about dma_handle?
> >>>>>>>>
> >>>>>>>> 1. Check what is the current running task in this GCE thread.
> >>>>>>>> 2. Yes.
> >>>>>>>> 3. Yes, CMDQ doesn't use iommu, so physical address is continuous.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Yes, physical addresses might be continous, but AFAIK there is no
> >>>>>>> guarantee that the dma_handle address is steadily growing, when
> >>>>>>> calling
> >>>>>>> dma_alloc_coherent. And if I understand the code correctly, you
> >>>>>>> use this
> >>>>>>> assumption to decide if the task picked from task_busy_list is
> >>>>>>> currently
> >>>>>>> executing. So I think this mecanism is not working.
> >>>>>>
> >>>>>> I don't use dma_handle address, and just use physical addresses.
> >>>>>>    From CPU's point of view, tasks are linked by the busy list.
> >>>>>>    From GCE's point of view, tasks are linked by the JUMP command.
> >>>>>>
> >>>>>>> In which cases does the HW thread raise an interrupt.
> >>>>>>> In case of error. When does CMDQ_THR_IRQ_DONE get raised?
> >>>>>>
> >>>>>> GCE will raise interrupt if any task is done or error.
> >>>>>> However, GCE is fast, so CPU may get multiple done tasks
> >>>>>> when it is running ISR.
> >>>>>>
> >>>>>> In case of error, that GCE thread will pause and raise interrupt.
> >>>>>> So, CPU may get multiple done tasks and one error task.
> >>>>>>
> >>>>>
> >>>>> I think we should reimplement the ISR mechanism. Can't we just read
> >>>>> CURR_IRQ_STATUS and THR_IRQ_STATUS in the handler and leave
> >>>>> cmdq_handle_error_done to the thread_fn? You will need to pass
> >>>>> information from the handler to thread_fn, but that shouldn't be an
> >>>>> issue. AFAIK interrupts are disabled in the handler, so we should stay
> >>>>> there as short as possible. Traversing task_busy_list is expensive, so
> >>>>> we need to do it in a thread context.
> >>>>
> >>>> Actually, our initial implementation is similar to your suggestion,
> >>>> but display needs CMDQ to return callback function very precisely,
> >>>> else display will drop frame.
> >>>> For display, CMDQ interrupt will be raised every 16 ~ 17 ms,
> >>>> and CMDQ needs to call callback function in ISR.
> >>>> If we defer callback to workqueue, the time interval may be larger than
> >>>> 32 ms.sometimes.
> >>>>
> >>>
> >>> I think the problem is, that you implemented the workqueue as a ordered
> >>> workqueue, so there is no parallel processing. I'm still not sure why
> >>> you need the workqueue to be ordered. Can you please explain.
> >>
> >> The order should be kept.
> >> Let me use mouse cursor as an example.
> >> If task 1 means move mouse cursor to point A, task 2 means point B,
> >> and task 3 means point C, our expected result is A -> B -> C.
> >> If the order is not kept, the result could become A -> C -> B.
> >>
> >
> > Got it, thanks for the clarification.
> >
> 
> I think a way to get rid of the workqueue is to use a timer, which gets 
> programmed to the time a timeout in the first task in the busy list 
> would happen. Everytime we update the busy list (e.g. because of task 
> got finished by the thread), we update the timer. When the timer 
> triggers, which hopefully won't happen too often, we return timeout on 
> the busy list elements, until the time is lower then the actual time.
> 
> At least with this we can reduce the data structures in this driver and 
> make it more lightweight.

>From my understanding, your proposed method can handle timeout case.

However, the workqueue is also in charge of releasing tasks.
Do you take releasing tasks into consideration by using the proposed
timer method?
Furthermore, I think the code will become more complex if we also use
timer to implement releasing tasks.

> Best regards,
> Matthias

Thanks,
HS

> >>>>> I keep thinking about how to get rid of the two data structures,
> >>>>> task_busy_list and the task_release_wq. We need the latter for the
> >>>>> only
> >>>>> sake of getting a timeout.
> >>>>>
> >>>>> Did you have a look on how the mailbox framework handles this?
> >>>>> By the way, what is the reason to not implement the whole driver as a
> >>>>> mailbox controller? For me, this driver looks like a good fit.
> >>>>
> >>>> CMDQ needs to encode commands for GCE hardware. We think this behavior
> >>>> should be put in CMDQ driver, and client just call CMDQ functions.
> >>>> Therefore, if we want to use mailbox framework, cmdq_rec must be
> >>>> mailbox client, and the others must be mailbox controller.
> >>>>
> >>>
> >>> You mean the functions to fill the cmdq_rec and execute it?
> >>> I think this should be part of the driver.
> >>
> >> Yes.
> >>
> >>> Jassi, can you have a look on the interface this driver exports [0].
> >>> They are needed to actually create the message which will be send.
> >>> Could something like this be part of a mailbox driver?
> >>>
> >>> [0] https://patchwork.kernel.org/patch/9140221/
> >>>
> >>>> However, if we use mailbox controller, CMDQ driver still needs to
> >>>> control busy list for each GCE thread, and use workqueue to handle
> >>>> timeout tasks.
> >>>>
> >>>
> >>> Let me summarize my ideas around this driver:
> >>> When we enter the ISR, we know that all task in task_busy_list before
> >>> the entry which represents curr_task can be set to TASK_STATE_DONE.
> >>> The curr_task could be TASK_STATE_ERROR if the corresponding bit in the
> >>> irq status registers is set.
> >>> Do we need to call the callback in the same order as the tasks got
> >>> dispatched to the HW thread? If not, we could try to call all this
> >>> callbacks in a multithreaded workqueue.
> >>
> >> Yes, we should keep order.
> >>
> >>> Regards,
> >>> Matthias
> >>
> >> Thanks,
> >> HS
> >>
> >>>> The only thing that we can borrow from mailbox framework is the send
> >>>> (CMDQ flush) and receive (CMDQ callback) interface, However, we don't
> >>>> think we can gain many benefits from it, and we have some overheads to
> >>>> conform to mailbox interface.
> >>>>
> >>>>
> >>>>>
> >>>>>>>>>> +            curr_task = task;
> >>>>>>>>>> +        if (task->cb.cb) {
> >>>>>>>>>> +            cmdq_cb_data.err = curr_task ? err : false;
> >>>>>>>>>> +            cmdq_cb_data.data = task->cb.data;
> >>>>>>>>>> +            task->cb.cb(cmdq_cb_data);
> >>>>>>>>>> +        }
> >>>>>>>>>> +        task->task_state = (curr_task && err) ?
> >>>>>>>>>> TASK_STATE_ERROR :
> >>>>>>>>>> +                TASK_STATE_DONE;
> >>>>>>>>>> +        list_del(&task->list_entry);
> >>>>>>>>>> +        if (curr_task)
> >>>>>>>>>> +            break;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    wake_up(&thread->wait_task_done);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static void cmdq_thread_irq_handler(struct cmdq *cmdq, int tid)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_thread *thread = &cmdq->thread[tid];
> >>>>>>>>>> +    unsigned long flags = 0L;
> >>>>>>>>>> +    u32 irq_flag;
> >>>>>>>>>> +
> >>>>>>>>>> +    spin_lock_irqsave(&cmdq->exec_lock, flags);
> >>>>>>>>>> +
> >>>>>>>>>> +    irq_flag = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
> >>>>>>>>>> +    cmdq_thread_writel(thread, ~irq_flag, CMDQ_THR_IRQ_STATUS);
> >>>>>>>>>> +
> >>>>>>>>>> +    /*
> >>>>>>>>>> +     * Another CPU core could run "release task" right before
> >>>>>>>>>> we acquire
> >>>>>>>>>> +     * the spin lock, and thus reset / disable this GCE
> >>>>>>>>>> thread, so we
> >>>>>>>>>> +     * need to check the enable bit of this GCE thread.
> >>>>>>>>>> +     */
> >>>>>>>>>> +    if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> >>>>>>>>>> +          CMDQ_THR_ENABLED))
> >>>>>>>>>> +        irq_flag = 0;
> >>>>>>>>>
> >>>>>>>>> cmdq_handle_error_done just retuns in this case. Programming
> >>>>>>>>> this way
> >>>>>>>>> just makes things confusing. What about:
> >>>>>>>>>
> >>>>>>>>> if (cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> >>>>>>>>>           CMDQ_THR_ENABLED)
> >>>>>>>>>     cmdq_handle_error_done(cmdq, thread, irq_flag);
> >>>>>>>>> else
> >>>>>>>>>         irq_flag = 0;
> >>>>>>>>>
> >>>>>>>>> spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>
> >>>>>>>> We still need to clear irq_flag if GCE thread is disabled.
> >>>>>>>> So, I think we can just return here.
> >>>>>>>>
> >>>>>>>>     if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> >>>>>>>>           CMDQ_THR_ENABLED))
> >>>>>>>>         return;
> >>>>>>>>
> >>>>>>>> What do you think?
> >>>>>>>>
> >>>>>>>
> >>>>>>> No, you can't just return, you need to unlock the spinlock.
> >>>>>>> Anyway I would prefer it the other way round, as I put it in my last
> >>>>>>> mail. Just delete the else branch, we don't need to set irq_flag
> >>>>>>> to zero.
> >>>>>>
> >>>>>> Sorry for my previous wrong reply since I merge your comment
> >>>>>> and CK's comment.
> >>>>>> http://lists.infradead.org/pipermail/linux-mediatek/2016-May/005547.html
> >>>>>>
> >>>>>> So, I will put this if condition into cmdq_task_handle_error_result()
> >>>>>> and then just return it if GCE thread is disabled.
> >>>>>>
> >>>>>
> >>>>> You mean in cmdq_task_handle_done
> >>>>> We should rename this functions to not create confusion.
> >>>>
> >>>> Sorry again. I mean in cmdq_handle_error_done().
> >>>> This function handles both done and error.
> >>>>
> >>>> I agree the function name looks confusion.
> >>>> I think it can be renamed to cmdq_thread_irq_handler()
> >>>> since it is used to handle irq for GCE thread.
> >>>>
> >>>>> Regards,
> >>>>> Matthias
> >>>>
> >>>> Thanks,
> >>>> HS
> >>>>
> >>>>>>>>>> +
> >>>>>>>>>> +    cmdq_handle_error_done(cmdq, thread, irq_flag);
> >>>>>>>>>> +    spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static irqreturn_t cmdq_irq_handler(int irq, void *dev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = dev;
> >>>>>>>>>> +    u32 irq_status;
> >>>>>>>>>> +    int i;
> >>>>>>>>>> +
> >>>>>>>>>> +    irq_status = readl(cmdq->base + CMDQ_CURR_IRQ_STATUS);
> >>>>>>>>>> +    irq_status &= CMDQ_IRQ_MASK;
> >>>>>>>>>> +    irq_status ^= CMDQ_IRQ_MASK;
> >>>>>>>>>
> >>>>>>>>> irq_status can be much bigger then 3, which is the number of
> >>>>>>>>> threads in
> >>>>>>>>> the system (CMDQ_THR_MAX_COUNT). So why we use this mask here
> >>>>>>>>> isn't
> >>>>>>>>> clear to me.
> >>>>>>>>
> >>>>>>>> Our GCE hardware has 16 threads, but we only use 3 threads
> >>>>>>>> currently.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Ok, but please use bitops here.
> >>>>>>
> >>>>>> Will use bitops.
> >>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +    if (!irq_status)
> >>>>>>>>>> +        return IRQ_NONE;
> >>>>>>>>>> +
> >>>>>>>>>> +    while (irq_status) {
> >>>>>>>>>> +        i = ffs(irq_status) - 1;
> >>>>>>>>>> +        irq_status &= ~BIT(i);
> >>>>>>>>>> +        cmdq_thread_irq_handler(cmdq, i);
> >>>>>>>>>> +    }
> >>>>>>>>>
> >>>>>>>>> Can you explain how the irq status register looks like, that
> >>>>>>>>> would it
> >>>>>>>>> make much easier to understand what happens here.
> >>>>>>>>
> >>>>>>>> Bit 0 ~ 15 of irq status register represents GCE thread 0 ~ 15
> >>>>>>>> interrupt. 0 means asserting interrupt; 1 means no interrupt.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Thanks, that helped. :)
> >>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +    return IRQ_HANDLED;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_task_handle_error_result(struct cmdq_task *task)
> >>>>>>>>>
> >>>>>>>>> We never check the return values, why do we have them?
> >>>>>>>>
> >>>>>>>> Will drop return value.
> >>>>>>>>
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = task->cmdq;
> >>>>>>>>>> +    struct device *dev = cmdq->dev;
> >>>>>>>>>> +    struct cmdq_thread *thread = task->thread;
> >>>>>>>>>> +    struct cmdq_task *next_task, *prev_task;
> >>>>>>>>>> +    u32 irq_flag;
> >>>>>>>>>> +
> >>>>>>>>>> +    /* suspend GCE thread to ensure consistency */
> >>>>>>>>>> +    WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> >>>>>>>>>> +
> >>>>>>>>>> +    /* ISR has handled this error task */
> >>>>>>>>>> +    if (task->task_state == TASK_STATE_ERROR) {
> >>>>>>>>>> +        next_task =
> >>>>>>>>>> list_first_entry_or_null(&thread->task_busy_list,
> >>>>>>>>>> +                struct cmdq_task, list_entry);
> >>>>>>>>>> +        if (next_task) /* move to next task */
> >>>>>>>>>> +            cmdq_thread_writel(thread, next_task->pa_base,
> >>>>>>>>>> +                       CMDQ_THR_CURR_ADDR);
> >>>>>>>>>
> >>>>>>>>> We have to do this, as we suppose that the thread did not reach
> >>>>>>>>> the jump
> >>>>>>>>> instruction we put into it's command queue, right?
> >>>>>>>>
> >>>>>>>> Yes.
> >>>>>>>>
> >>>>>>>
> >>>>>>> So this should then go into it's own function. In wait_release_work,
> >>>>>>> something like this:
> >>>>>>>
> >>>>>>> if(task->task_state == TASK_STATE_ERROR)
> >>>>>>>          cmdq_task_handle_error(task)
> >>>>>>
> >>>>>> OK.
> >>>>>> I will write new function cmdq_task_handle_error() to handle error
> >>>>>> case.
> >>>>>>
> >>>>>>>>>> +        cmdq_thread_resume(thread);
> >>>>>>>>>> +        return -ECANCELED;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> if task_state != ERROR and != DONE. This means that the timeout of
> >>>>>>>>> task_release_wq has timed out, right?
> >>>>>>>>
> >>>>>>>> Yes.
> >>>>>>>>
> >>>>>>>>>> +    /*
> >>>>>>>>>> +     * Save next_task and prev_task in advance
> >>>>>>>>>> +     * since cmdq_handle_error_done will remove list_entry.
> >>>>>>>>>> +     */
> >>>>>>>>>> +    next_task = prev_task = NULL;
> >>>>>>>>>> +    if (task->list_entry.next != &thread->task_busy_list)
> >>>>>>>>>> +        next_task = list_next_entry(task, list_entry);
> >>>>>>>>>> +    if (task->list_entry.prev != &thread->task_busy_list)
> >>>>>>>>>> +        prev_task = list_prev_entry(task, list_entry);
> >>>>>>>>>> +
> >>>>>>>>>> +    /*
> >>>>>>>>>> +     * Although IRQ is disabled, GCE continues to execute.
> >>>>>>>>>> +     * It may have pending IRQ before GCE thread is suspended,
> >>>>>>>>>> +     * so check this condition again.
> >>>>>>>>>> +     */
> >>>>>>>>>
> >>>>>>>>> The first thing we did in this function was suspending the
> >>>>>>>>> thread. Why
> >>>>>>>>> do we need this then?
> >>>>>>>>
> >>>>>>>> Because timeout is CPU timeout not GCE timeout, GCE could just
> >>>>>>>> finish
> >>>>>>>> this task before the GCE thread is suspended.
> >>>>>>>>
> >>>>>>>
> >>>>>>> What are the reasons for a timeout? An error has happend, or the
> >>>>>>> task is
> >>>>>>> still executing.
> >>>>>>
> >>>>>>    From GCE's point of view, this task is still executing.
> >>>>>> But, it could be an error of client.
> >>>>>> For example, task may never get event if display turn off hardware
> >>>>>> before waiting for task to finish its work.
> >>>>>>
> >>>>>>>>> To be honest this whole functions looks really like a design
> >>>>>>>>> error. We
> >>>>>>>>> have to sperate the states much clearer so that it is possible to
> >>>>>>>>> understand what is happening in the GCE. Isn't it for example
> >>>>>>>>> posible to
> >>>>>>>>> have worker queues for timed out tasks and tasks with an error?
> >>>>>>>>> I'm not
> >>>>>>>>> sure how to do this, actually I'm not sure if I really
> >>>>>>>>> understood how
> >>>>>>>>> this is supposed to work.
> >>>>>>>>
> >>>>>>>> GCE doesn't have timeout. The timeout is decided and controlled
> >>>>>>>> by CPU,
> >>>>>>>> so we check timeout in release work.
> >>>>>>>> For error and done, they are easy to check by register, and we have
> >>>>>>>> already created release work for timeout. So, I don't think we
> >>>>>>>> need to
> >>>>>>>> create work queue for each case.
> >>>>>>>>
> >>>>>>>> What do you think?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I think, if we find in here, that the irq_flag is set, then the the
> >>>>>>> interrupt handler was triggered and is spinning the spinlock. If
> >>>>>>> this is
> >>>>>>> not the case, we have a timeout and we handle this.
> >>>>>>
> >>>>>> I will write another function to handle error, and handle timeout
> >>>>>> here.
> >>>>>>
> >>>>>>>>> How much do we win, when we patch the thread command queue for
> >>>>>>>>> every
> >>>>>>>>> task we add, instead of just taking one task after another from
> >>>>>>>>> the
> >>>>>>>>> task_busy_list?
> >>>>>>>>
> >>>>>>>> GCE is used to help read/write registers with critical time
> >>>>>>>> limitation.
> >>>>>>>> Sometimes, client may ask to process multiple tasks in a short
> >>>>>>>> period
> >>>>>>>> of time, e.g. display flush multiple tasks for next vblank. So,
> >>>>>>>> CMDQ
> >>>>>>>> shouldn't limit to process one task after another from the
> >>>>>>>> task_busy_list. Currently, when interrupt or timeout, we will check
> >>>>>>>> how many tasks are done, and which one is error or timeout.
> >>>>>>>>
> >>>>>>>
> >>>>>>> So I suppose the device driver who use this are interested in
> >>>>>>> throughput
> >>>>>>> and not in latency. The callback of every
> >>>>>>>
> >>>>>>>>>> +    irq_flag = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
> >>>>>>>>>> +    cmdq_handle_error_done(cmdq, thread, irq_flag);
> >>>>>>>>>> +    cmdq_thread_writel(thread, ~irq_flag, CMDQ_THR_IRQ_STATUS);
> >>>>>>>>>> +
> >>>>>>>>>> +    if (task->task_state == TASK_STATE_DONE) {
> >>>>>>>>>> +        cmdq_thread_resume(thread);
> >>>>>>>>>> +        return 0;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    if (task->task_state == TASK_STATE_ERROR) {
> >>>>>>>>>> +        dev_err(dev, "task 0x%p error\n", task);
> >>>>>>>>>> +        if (next_task) /* move to next task */
> >>>>>>>>>> +            cmdq_thread_writel(thread, next_task->pa_base,
> >>>>>>>>>> +                       CMDQ_THR_CURR_ADDR);
> >>>>>>>>>> +        cmdq_thread_resume(thread);
> >>>>>>>>>> +        return -ECANCELED;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    /* Task is running, so we force to remove it. */
> >>>>>>>>>> +    dev_err(dev, "task 0x%p timeout or killed\n", task);
> >>>>>>>>>> +    task->task_state = TASK_STATE_ERROR;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (prev_task) {
> >>>>>>>>>> +        u64 *prev_va = prev_task->va_base;
> >>>>>>>>>> +        u64 *curr_va = task->va_base;
> >>>>>>>>>> +
> >>>>>>>>>> +        /* copy JUMP instruction */
> >>>>>>>>>> +        prev_va[prev_task->num_cmd - 1] =
> >>>>>>>>>> curr_va[task->num_cmd - 1];
> >>>>>>>>>> +
> >>>>>>>>>> +        cmdq_thread_invalidate_fetched_data(thread);
> >>>>>>>>>> +    } else if (next_task) { /* move to next task */
> >>>>>>>>>> +        cmdq_thread_writel(thread, next_task->pa_base,
> >>>>>>>>>> +                   CMDQ_THR_CURR_ADDR);
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    list_del(&task->list_entry);
> >>>>>>>>>> +    cmdq_thread_resume(thread);
> >>>>>>>>>> +
> >>>>>>>>>> +    /* call cb here to prevent lock */
> >>>>>>>>>> +    if (task->cb.cb) {
> >>>>>>>>>> +        struct cmdq_cb_data cmdq_cb_data;
> >>>>>>>>>> +
> >>>>>>>>>> +        cmdq_cb_data.err = true;
> >>>>>>>>>> +        cmdq_cb_data.data = task->cb.data;
> >>>>>>>>>> +        task->cb.cb(cmdq_cb_data);
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    return -ECANCELED;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static void cmdq_task_wait_release_work(struct work_struct
> >>>>>>>>>> *work_item)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_task *task = container_of(work_item, struct
> >>>>>>>>>> cmdq_task,
> >>>>>>>>>> +                          release_work);
> >>>>>>>>>> +    struct cmdq *cmdq = task->cmdq;
> >>>>>>>>>> +    struct cmdq_thread *thread = task->thread;
> >>>>>>>>>> +    unsigned long flags;
> >>>>>>>>>> +
> >>>>>>>>>> +    wait_event_timeout(thread->wait_task_done,
> >>>>>>>>>> +               task->task_state != TASK_STATE_BUSY,
> >>>>>>>>>> +               msecs_to_jiffies(CMDQ_TIMEOUT_MS));
> >>>>>>>>>> +
> >>>>>>>>>> +    spin_lock_irqsave(&cmdq->exec_lock, flags);
> >>>>>>>>>> +    if (task->task_state != TASK_STATE_DONE)
> >>>>>>>>>> +        cmdq_task_handle_error_result(task);
> >>>>>>>>>> +    if (list_empty(&thread->task_busy_list))
> >>>>>>>>>> +        cmdq_thread_disable(cmdq, thread);
> >>>>>>>>>> +    spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>>> +
> >>>>>>>>>> +    /* release regardless of success or not */
> >>>>>>>>>> +    clk_disable_unprepare(cmdq->clock);
> >>>>>>>>>> +    cmdq_task_release(task);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static void cmdq_task_wait_release_schedule(struct cmdq_task
> >>>>>>>>>> *task)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = task->cmdq;
> >>>>>>>>>> +
> >>>>>>>>>> +    INIT_WORK(&task->release_work, cmdq_task_wait_release_work);
> >>>>>>>>>> +    queue_work(cmdq->task_release_wq, &task->release_work);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_rec_realloc_cmd_buffer(struct cmdq_rec *rec,
> >>>>>>>>>> size_t size)
> >>>>>>>>>> +{
> >>>>>>>>>> +    void *new_buf;
> >>>>>>>>>> +
> >>>>>>>>>> +    new_buf = krealloc(rec->buf, size, GFP_KERNEL | __GFP_ZERO);
> >>>>>>>>>> +    if (!new_buf)
> >>>>>>>>>> +        return -ENOMEM;
> >>>>>>>>>> +    rec->buf = new_buf;
> >>>>>>>>>> +    rec->buf_size = size;
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +struct cmdq_base *cmdq_register_device(struct device *dev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_base *cmdq_base;
> >>>>>>>>>> +    struct resource res;
> >>>>>>>>>> +    int subsys;
> >>>>>>>>>> +    u32 base;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (of_address_to_resource(dev->of_node, 0, &res))
> >>>>>>>>>> +        return NULL;
> >>>>>>>>>> +    base = (u32)res.start;
> >>>>>>>>>> +
> >>>>>>>>>> +    subsys = cmdq_subsys_base_to_id(base >> 16);
> >>>>>>>>>> +    if (subsys < 0)
> >>>>>>>>>> +        return NULL;
> >>>>>>>>>> +
> >>>>>>>>>> +    cmdq_base = devm_kmalloc(dev, sizeof(*cmdq_base),
> >>>>>>>>>> GFP_KERNEL);
> >>>>>>>>>> +    if (!cmdq_base)
> >>>>>>>>>> +        return NULL;
> >>>>>>>>>> +    cmdq_base->subsys = subsys;
> >>>>>>>>>> +    cmdq_base->base = base;
> >>>>>>>>>> +
> >>>>>>>>>> +    return cmdq_base;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_register_device);
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_create(struct device *dev, u64 engine_flag,
> >>>>>>>>>> +            struct cmdq_rec **rec_ptr)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_rec *rec;
> >>>>>>>>>> +    int err;
> >>>>>>>>>> +
> >>>>>>>>>> +    rec = kzalloc(sizeof(*rec), GFP_KERNEL);
> >>>>>>>>>> +    if (!rec)
> >>>>>>>>>> +        return -ENOMEM;
> >>>>>>>>>> +    rec->cmdq = dev_get_drvdata(dev);
> >>>>>>>>>> +    rec->engine_flag = engine_flag;
> >>>>>>>>>> +    err = cmdq_rec_realloc_cmd_buffer(rec,
> >>>>>>>>>> CMDQ_INITIAL_CMD_BLOCK_SIZE);
> >>>>>>>>>
> >>>>>>>>> Just pass PAGE_SIZE here, no need for CMDQ_INITIAL_CMD_BLOCK_SIZE.
> >>>>>>>>
> >>>>>>>> Will do.
> >>>>>>>>
> >>>>>>>>>> +    if (err < 0) {
> >>>>>>>>>> +        kfree(rec);
> >>>>>>>>>> +        return err;
> >>>>>>>>>> +    }
> >>>>>>>>>> +    *rec_ptr = rec;
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_create);
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_rec_append_command(struct cmdq_rec *rec, enum
> >>>>>>>>>> cmdq_code code,
> >>>>>>>>>> +                   u32 arg_a, u32 arg_b)
> >>>>>>>>>> +{
> >>>>>>>>>> +    u64 *cmd_ptr;
> >>>>>>>>>> +    int err;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (WARN_ON(rec->finalized))
> >>>>>>>>>> +        return -EBUSY;
> >>>>>>>>>> +    if (code < CMDQ_CODE_MOVE || code > CMDQ_CODE_EOC)
> >>>>>>>>>> +        return -EINVAL;
> >>>>>>>>>
> >>>>>>>>> cmdq_rec_append_command is just called from inside this driver
> >>>>>>>>> and code
> >>>>>>>>> is a enum. We can expect it to be correct, no need for this check.
> >>>>>>>>
> >>>>>>>> Will drop this check.
> >>>>>>>>
> >>>>>>>>>> +    if (unlikely(rec->command_size + CMDQ_INST_SIZE >
> >>>>>>>>>> rec->buf_size)) {
> >>>>>>>>>
> >>>>>>>>> command_size is the offset into the buffer to which a new
> >>>>>>>>> command is
> >>>>>>>>> written, so this name is highly confusing. I wonder if this
> >>>>>>>>> would be
> >>>>>>>>> easier to understand if we redefine command_size to something
> >>>>>>>>> like the
> >>>>>>>>> number of commands and divide/multiply CMDQ_INST_SIZE where
> >>>>>>>>> this is needed.
> >>>>>>>>
> >>>>>>>> I can rename command_size to cmd_buf_size and calculate num_cmd by
> >>>>>>>> dividing CMDQ_INST_SIZE.
> >>>>>>>> What do you think?
> >>>>>>>>
> >>>>>>>>>> +        err = cmdq_rec_realloc_cmd_buffer(rec, rec->buf_size
> >>>>>>>>>> * 2);
> >>>>>>>>>> +        if (err < 0)
> >>>>>>>>>> +            return err;
> >>>>>>>>>> +    }
> >>>>>>>>>> +    cmd_ptr = rec->buf + rec->command_size;
> >>>>>>>>>> +    (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a)
> >>>>>>>>>> << 32 | arg_b;
> >>>>>>>>>> +    rec->command_size += CMDQ_INST_SIZE;
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_write(struct cmdq_rec *rec, u32 value, struct
> >>>>>>>>>> cmdq_base *base,
> >>>>>>>>>> +           u32 offset)
> >>>>>>>>>> +{
> >>>>>>>>>> +    u32 arg_a = ((base->base + offset) &
> >>>>>>>>>> CMDQ_ARG_A_WRITE_MASK) |
> >>>>>>>>>> +            ((base->subsys & CMDQ_SUBSYS_MASK) <<
> >>>>>>>>>> CMDQ_SUBSYS_SHIFT);
> >>>>>>>>>
> >>>>>>>>> base->subsys is the id from gce_sybsys, so we can expect it to be
> >>>>>>>>> correct, no need to mask with CMDQ_SUBSYS_MASK.
> >>>>>>>>
> >>>>>>>> Will drop it.
> >>>>>>>>
> >>>>>>>>>> +    return cmdq_rec_append_command(rec, CMDQ_CODE_WRITE,
> >>>>>>>>>> arg_a, value);
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_write);
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_write_mask(struct cmdq_rec *rec, u32 value,
> >>>>>>>>>> +            struct cmdq_base *base, u32 offset, u32 mask)
> >>>>>>>>>> +{
> >>>>>>>>>> +    u32 offset_mask = offset;
> >>>>>>>>>> +    int err;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (mask != 0xffffffff) {
> >>>>>>>>>> +        err = cmdq_rec_append_command(rec, CMDQ_CODE_MOVE, 0,
> >>>>>>>>>> ~mask);
> >>>>>>>>>> +        if (err < 0)
> >>>>>>>>>> +            return err;
> >>>>>>>>>> +        offset_mask |= CMDQ_WRITE_ENABLE_MASK;
> >>>>>>>>>> +    }
> >>>>>>>>>> +    return cmdq_rec_write(rec, value, base, offset_mask);
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_write_mask);
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_wfe(struct cmdq_rec *rec, enum cmdq_event event)
> >>>>>>>>>> +{
> >>>>>>>>>> +    u32 arg_b;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (event >= CMDQ_MAX_HW_EVENT_COUNT || event < 0)
> >>>>>>>>>> +        return -EINVAL;
> >>>>>>>>>> +
> >>>>>>>>>> +    /*
> >>>>>>>>>> +     * bit 0-11: wait value
> >>>>>>>>>> +     * bit 15: 1 - wait, 0 - no wait
> >>>>>>>>>> +     * bit 16-27: update value
> >>>>>>>>>> +     * bit 31: 1 - update, 0 - no update
> >>>>>>>>>> +     */
> >>>>>>>>>
> >>>>>>>>> I don't understand this comments. What are they for?
> >>>>>>>>
> >>>>>>>> This is for WFE command. I will comment it.
> >>>>>>>>
> >>>>>>>>>> +    arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT |
> >>>>>>>>>> CMDQ_WFE_WAIT_VALUE;
> >>>>>>>>>> +    return cmdq_rec_append_command(rec, CMDQ_CODE_WFE, event,
> >>>>>>>>>> arg_b);
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_wfe);
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_clear_event(struct cmdq_rec *rec, enum
> >>>>>>>>>> cmdq_event event)
> >>>>>>>>>> +{
> >>>>>>>>>> +    if (event >= CMDQ_MAX_HW_EVENT_COUNT || event < 0)
> >>>>>>>>>> +        return -EINVAL;
> >>>>>>>>>> +
> >>>>>>>>>> +    return cmdq_rec_append_command(rec, CMDQ_CODE_WFE, event,
> >>>>>>>>>> +                       CMDQ_WFE_UPDATE);
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_clear_event);
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_rec_finalize(struct cmdq_rec *rec)
> >>>>>>>>>> +{
> >>>>>>>>>> +    int err;
> >>>>>>>>>> +
> >>>>>>>>>> +    if (rec->finalized)
> >>>>>>>>>> +        return 0;
> >>>>>>>>>> +
> >>>>>>>>>> +    /* insert EOC and generate IRQ for each command iteration */
> >>>>>>>>>> +    err = cmdq_rec_append_command(rec, CMDQ_CODE_EOC, 0,
> >>>>>>>>>> CMDQ_EOC_IRQ_EN);
> >>>>>>>>>> +    if (err < 0)
> >>>>>>>>>> +        return err;
> >>>>>>>>>> +
> >>>>>>>>>> +    /* JUMP to end */
> >>>>>>>>>> +    err = cmdq_rec_append_command(rec, CMDQ_CODE_JUMP, 0,
> >>>>>>>>>> CMDQ_JUMP_PASS);
> >>>>>>>>>> +    if (err < 0)
> >>>>>>>>>> +        return err;
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> Does this need to be atomic?
> >>>>>>>>> What happens if after CODE_EOC and before CODE_JUMP some
> >>>>>>>>> write/read/event gets added?
> >>>>>>>>> What happens if more commands get added to the queue after
> >>>>>>>>> CODE_JUMP,
> >>>>>>>>> but before finalized is set to true. Why don't you use atomic
> >>>>>>>>> functions
> >>>>>>>>> to access finalized?
> >>>>>>>>
> >>>>>>>> Since cmdq_rec doesn't guarantee thread safe, mutex is needed when
> >>>>>>>> client uses cmdq_rec.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Well I think that rec->finalized tries to implement this, but might
> >>>>>>> fail, if two kernel threads work on the same cmdq_rec.
> >>>>>>>
> >>>>>>>>>> +    rec->finalized = true;
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_flush_async(struct cmdq_rec *rec,
> >>>>>>>>>> cmdq_async_flush_cb cb,
> >>>>>>>>>> +             void *data)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = rec->cmdq;
> >>>>>>>>>> +    struct cmdq_task *task;
> >>>>>>>>>> +    struct cmdq_task_cb task_cb;
> >>>>>>>>>> +    struct cmdq_thread *thread;
> >>>>>>>>>> +    int err;
> >>>>>>>>>> +
> >>>>>>>>>> +    mutex_lock(&cmdq->task_mutex);
> >>>>>>>>>> +    if (cmdq->suspended) {
> >>>>>>>>>> +        dev_err(cmdq->dev, "%s is called after suspended\n",
> >>>>>>>>>> __func__);
> >>>>>>>>>> +        mutex_unlock(&cmdq->task_mutex);
> >>>>>>>>>> +        return -EPERM;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    err = cmdq_rec_finalize(rec);
> >>>>>>>>>> +    if (err < 0) {
> >>>>>>>>>> +        mutex_unlock(&cmdq->task_mutex);
> >>>>>>>>>> +        return err;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    task_cb.cb = cb;
> >>>>>>>>>> +    task_cb.data = data;
> >>>>>>>>>> +    task = cmdq_task_acquire(rec, task_cb);
> >>>>>>>>>> +    if (!task) {
> >>>>>>>>>> +        mutex_unlock(&cmdq->task_mutex);
> >>>>>>>>>> +        return -EFAULT;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    thread =
> >>>>>>>>>> &cmdq->thread[cmdq_eng_get_thread(task->engine_flag)];
> >>>>>>>>>> +    cmdq_task_exec(task, thread);
> >>>>>>>>>> +    cmdq_task_wait_release_schedule(task);
> >>>>>>>>>> +    mutex_unlock(&cmdq->task_mutex);
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_flush_async);
> >>>>>>>>>> +
> >>>>>>>>>> +struct cmdq_flush_completion {
> >>>>>>>>>> +    struct completion cmplt;
> >>>>>>>>>> +    bool err;
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_rec_flush_cb(struct cmdq_cb_data data)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_flush_completion *cmplt = data.data;
> >>>>>>>>>> +
> >>>>>>>>>> +    cmplt->err = data.err;
> >>>>>>>>>> +    complete(&cmplt->cmplt);
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +int cmdq_rec_flush(struct cmdq_rec *rec)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_flush_completion cmplt;
> >>>>>>>>>> +    int err;
> >>>>>>>>>> +
> >>>>>>>>>> +    init_completion(&cmplt.cmplt);
> >>>>>>>>>> +    err = cmdq_rec_flush_async(rec, cmdq_rec_flush_cb, &cmplt);
> >>>>>>>>>> +    if (err < 0)
> >>>>>>>>>> +        return err;
> >>>>>>>>>> +    wait_for_completion(&cmplt.cmplt);
> >>>>>>>>>> +    return cmplt.err ? -EFAULT : 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_flush);
> >>>>>>>>>> +
> >>>>>>>>>> +void cmdq_rec_destroy(struct cmdq_rec *rec)
> >>>>>>>>>> +{
> >>>>>>>>>> +    kfree(rec->buf);
> >>>>>>>>>> +    kfree(rec);
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL(cmdq_rec_destroy);
> >>>>>>>>>> +
> >>>>>>>>>> +static bool cmdq_task_is_empty(struct cmdq *cmdq)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq_thread *thread;
> >>>>>>>>>> +    int i;
> >>>>>>>>>> +
> >>>>>>>>>> +    for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> >>>>>>>>>> +        thread = &cmdq->thread[i];
> >>>>>>>>>> +        if (!list_empty(&thread->task_busy_list))
> >>>>>>>>>> +            return false;
> >>>>>>>>>> +    }
> >>>>>>>>>> +    return true;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_suspend(struct device *dev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = dev_get_drvdata(dev);
> >>>>>>>>>> +    u32 exec_threads;
> >>>>>>>>>> +
> >>>>>>>>>> +    mutex_lock(&cmdq->task_mutex);
> >>>>>>>>>> +    cmdq->suspended = true;
> >>>>>>>>>> +    mutex_unlock(&cmdq->task_mutex);
> >>>>>>>>>> +
> >>>>>>>>>> +    exec_threads = readl(cmdq->base + CMDQ_CURR_LOADED_THR);
> >>>>>>>>>> +    if ((exec_threads & CMDQ_THR_EXECUTING) &&
> >>>>>>>>>> !cmdq_task_is_empty(cmdq)) {
> >>>>>>>>>> +        dev_err(dev, "wait active tasks timeout.\n");
> >>>>>>>>>> +        flush_workqueue(cmdq->task_release_wq);
> >>>>>>>>>> +    }
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_resume(struct device *dev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = dev_get_drvdata(dev);
> >>>>>>>>>> +
> >>>>>>>>>> +    cmdq->suspended = false;
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_remove(struct platform_device *pdev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct cmdq *cmdq = platform_get_drvdata(pdev);
> >>>>>>>>>> +
> >>>>>>>>>> +    destroy_workqueue(cmdq->task_release_wq);
> >>>>>>>>>> +    cmdq->task_release_wq = NULL;
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static int cmdq_probe(struct platform_device *pdev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct device *dev = &pdev->dev;
> >>>>>>>>>> +    struct device_node *node = dev->of_node;
> >>>>>>>>>> +    struct resource *res;
> >>>>>>>>>> +    struct cmdq *cmdq;
> >>>>>>>>>> +    int err, i;
> >>>>>>>>>> +
> >>>>>>>>>> +    cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
> >>>>>>>>>> +    if (!cmdq)
> >>>>>>>>>> +        return -ENOMEM;
> >>>>>>>>>> +    cmdq->dev = dev;
> >>>>>>>>>> +
> >>>>>>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>>>>>>>> +    cmdq->base = devm_ioremap_resource(dev, res);
> >>>>>>>>>> +    if (IS_ERR(cmdq->base)) {
> >>>>>>>>>> +        dev_err(dev, "failed to ioremap gce\n");
> >>>>>>>>>> +        return PTR_ERR(cmdq->base);
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    cmdq->irq = irq_of_parse_and_map(node, 0);
> >>>>>>>>>> +    if (!cmdq->irq) {
> >>>>>>>>>> +        dev_err(dev, "failed to get irq\n");
> >>>>>>>>>> +        return -EINVAL;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    dev_dbg(dev, "cmdq device: addr:0x%p, va:0x%p, irq:%d\n",
> >>>>>>>>>> +        dev, cmdq->base, cmdq->irq);
> >>>>>>>>>> +
> >>>>>>>>>> +    mutex_init(&cmdq->task_mutex);
> >>>>>>>>>> +    spin_lock_init(&cmdq->exec_lock);
> >>>>>>>>>> +    cmdq->task_release_wq = alloc_ordered_workqueue(
> >>>>>>>>>> +            "%s", WQ_MEM_RECLAIM | WQ_HIGHPRI,
> >>>>>>>>>> +            "cmdq_task_wait_release");
> >>>>>>>>>> +
> >>>>>>>>>> +    for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> >>>>>>>>>> +        cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> >>>>>>>>>> +                CMDQ_THR_SHIFT * i;
> >>>>>>>>>> +        init_waitqueue_head(&cmdq->thread[i].wait_task_done);
> >>>>>>>>>> +        INIT_LIST_HEAD(&cmdq->thread[i].task_busy_list);
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    platform_set_drvdata(pdev, cmdq);
> >>>>>>>>>> +
> >>>>>>>>>> +    err = devm_request_irq(dev, cmdq->irq, cmdq_irq_handler,
> >>>>>>>>>> IRQF_SHARED,
> >>>>>>>>>> +                   CMDQ_DRIVER_DEVICE_NAME, cmdq);
> >>>>>>>>>> +    if (err < 0) {
> >>>>>>>>>> +        dev_err(dev, "failed to register ISR (%d)\n", err);
> >>>>>>>>>> +        goto fail;
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    cmdq->clock = devm_clk_get(dev, CMDQ_CLK_NAME);
> >>>>>>>>>> +    if (IS_ERR(cmdq->clock)) {
> >>>>>>>>>> +        dev_err(dev, "failed to get clk:%s\n", CMDQ_CLK_NAME);
> >>>>>>>>>> +        err = PTR_ERR(cmdq->clock);
> >>>>>>>>>> +        goto fail;
> >>>>>>>>>> +    }
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +
> >>>>>>>>>> +fail:
> >>>>>>>>>> +    cmdq_remove(pdev);
> >>>>>>>>>> +    return err;
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static const struct dev_pm_ops cmdq_pm_ops = {
> >>>>>>>>>> +    .suspend = cmdq_suspend,
> >>>>>>>>>> +    .resume = cmdq_resume,
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +static const struct of_device_id cmdq_of_ids[] = {
> >>>>>>>>>> +    {.compatible = "mediatek,mt8173-gce",},
> >>>>>>>>>> +    {}
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +static struct platform_driver cmdq_drv = {
> >>>>>>>>>> +    .probe = cmdq_probe,
> >>>>>>>>>> +    .remove = cmdq_remove,
> >>>>>>>>>> +    .driver = {
> >>>>>>>>>> +        .name = CMDQ_DRIVER_DEVICE_NAME,
> >>>>>>>>>> +        .owner = THIS_MODULE,
> >>>>>>>>>> +        .pm = &cmdq_pm_ops,
> >>>>>>>>>> +        .of_match_table = cmdq_of_ids,
> >>>>>>>>>> +    }
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +builtin_platform_driver(cmdq_drv);
> >>>>>>>>>> diff --git a/include/soc/mediatek/cmdq.h
> >>>>>>>>>> b/include/soc/mediatek/cmdq.h
> >>>>>>>>>> new file mode 100644
> >>>>>>>>>> index 0000000..60eef3d
> >>>>>>>>>> --- /dev/null
> >>>>>>>>>> +++ b/include/soc/mediatek/cmdq.h
> >>>>>>>>>> @@ -0,0 +1,197 @@
> >>>>>>>>>> +/*
> >>>>>>>>>> + * Copyright (c) 2015 MediaTek Inc.
> >>>>>>>>>> + *
> >>>>>>>>>> + * This program is free software; you can redistribute it
> >>>>>>>>>> and/or modify
> >>>>>>>>>> + * it under the terms of the GNU General Public License
> >>>>>>>>>> version 2 as
> >>>>>>>>>> + * published by the Free Software Foundation.
> >>>>>>>>>> + *
> >>>>>>>>>> + * This program is distributed in the hope that it will be
> >>>>>>>>>> useful,
> >>>>>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied
> >>>>>>>>>> warranty of
> >>>>>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>>>>>>>> + * GNU General Public License for more details.
> >>>>>>>>>> + */
> >>>>>>>>>> +
> >>>>>>>>>> +#ifndef __MTK_CMDQ_H__
> >>>>>>>>>> +#define __MTK_CMDQ_H__
> >>>>>>>>>> +
> >>>>>>>>>> +#include <linux/platform_device.h>
> >>>>>>>>>> +#include <linux/types.h>
> >>>>>>>>>> +
> >>>>>>>>>> +enum cmdq_eng {
> >>>>>>>>>> +    CMDQ_ENG_DISP_AAL,
> >>>>>>>>>> +    CMDQ_ENG_DISP_COLOR0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_COLOR1,
> >>>>>>>>>> +    CMDQ_ENG_DISP_DPI0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_DSI0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_DSI1,
> >>>>>>>>>> +    CMDQ_ENG_DISP_GAMMA,
> >>>>>>>>>> +    CMDQ_ENG_DISP_OD,
> >>>>>>>>>> +    CMDQ_ENG_DISP_OVL0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_OVL1,
> >>>>>>>>>> +    CMDQ_ENG_DISP_PWM0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_PWM1,
> >>>>>>>>>> +    CMDQ_ENG_DISP_RDMA0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_RDMA1,
> >>>>>>>>>> +    CMDQ_ENG_DISP_RDMA2,
> >>>>>>>>>> +    CMDQ_ENG_DISP_UFOE,
> >>>>>>>>>> +    CMDQ_ENG_DISP_WDMA0,
> >>>>>>>>>> +    CMDQ_ENG_DISP_WDMA1,
> >>>>>>>>>> +    CMDQ_ENG_MAX,
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +/* events for CMDQ and display */
> >>>>>>>>>> +enum cmdq_event {
> >>>>>>>>>> +    /* Display start of frame(SOF) events */
> >>>>>>>>>> +    CMDQ_EVENT_DISP_OVL0_SOF = 11,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_OVL1_SOF = 12,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA0_SOF = 13,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA1_SOF = 14,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA2_SOF = 15,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_WDMA0_SOF = 16,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_WDMA1_SOF = 17,
> >>>>>>>>>> +    /* Display end of frame(EOF) events */
> >>>>>>>>>> +    CMDQ_EVENT_DISP_OVL0_EOF = 39,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_OVL1_EOF = 40,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA0_EOF = 41,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA1_EOF = 42,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA2_EOF = 43,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_WDMA0_EOF = 44,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_WDMA1_EOF = 45,
> >>>>>>>>>> +    /* Mutex end of frame(EOF) events */
> >>>>>>>>>> +    CMDQ_EVENT_MUTEX0_STREAM_EOF = 53,
> >>>>>>>>>> +    CMDQ_EVENT_MUTEX1_STREAM_EOF = 54,
> >>>>>>>>>> +    CMDQ_EVENT_MUTEX2_STREAM_EOF = 55,
> >>>>>>>>>> +    CMDQ_EVENT_MUTEX3_STREAM_EOF = 56,
> >>>>>>>>>> +    CMDQ_EVENT_MUTEX4_STREAM_EOF = 57,
> >>>>>>>>>> +    /* Display underrun events */
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA0_UNDERRUN = 63,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA1_UNDERRUN = 64,
> >>>>>>>>>> +    CMDQ_EVENT_DISP_RDMA2_UNDERRUN = 65,
> >>>>>>>>>> +    /* Keep this at the end of HW events */
> >>>>>>>>>> +    CMDQ_MAX_HW_EVENT_COUNT = 260,
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +struct cmdq_cb_data {
> >>>>>>>>>> +    bool    err;
> >>>>>>>>>> +    void    *data;
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +typedef int (*cmdq_async_flush_cb)(struct cmdq_cb_data data);
> >>>>>>>>>> +
> >>>>>>>>>> +struct cmdq_task;
> >>>>>>>>>> +struct cmdq;
> >>>>>>>>>> +
> >>>>>>>>>> +struct cmdq_rec {
> >>>>>>>>>> +    struct cmdq        *cmdq;
> >>>>>>>>>> +    u64            engine_flag;
> >>>>>>>>>> +    size_t            command_size;
> >>>>>>>>>> +    void            *buf;
> >>>>>>>>>> +    size_t            buf_size;
> >>>>>>>>>> +    bool            finalized;
> >>>>>>>>>> +};
> >>>>>>>
> >>>>>>> Why do we need cmdq_rec at all? Can't we just use the cmdq_task
> >>>>>>> for that
> >>>>>>> and this way make the driver less complex?
> >>>>>>
> >>>>>> There are two main reasons for cmdq_rec.
> >>>>>> 1. It is slow to access dma too frequently.
> >>>>>>       So, we append commands to cacheable memory, and then flush
> >>>>>> to dma.
> >>>>>> 2. cmdq_rec is not thread safe, but cmdq_task needs thread safe.
> >>>>>>       If we merge them, we need to take care of some synchronization
> >>>>>>       issues.
> >>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +struct cmdq_base {
> >>>>>>>>>> +    int    subsys;
> >>>>>>>>>> +    u32    base;
> >>>>>>>>>
> >>>>>>>>> subsys can always be calculated via cmdq_subsys_base_to_id(base
> >>>>>>>>> >> 16)
> >>>>>>>>> so we can get rid of the struct, right?
> >>>>>>>>
> >>>>>>>> Current subsys method is based on previous comment from Daniel
> >>>>>>>> Kurtz.
> >>>>>>>> Please take a look of our previous discussion.
> >>>>>>>> http://lists.infradead.org/pipermail/linux-mediatek/2016-February/004483.html
> >>>>>>>>
> >>>>>>>> Thanks.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I have to look deeper into this, but from what I read, the
> >>>>>>> proposal from
> >>>>>>> Daniel [1] seems good to me.
> >>>>>>>
> >>>>>>> [1] https://patchwork.kernel.org/patch/8068311/
> >>>>>>
> >>>>>> The initial proposal has some problem, so please see the follow-up
> >>>>>> discussions. Thanks.
> >>>>>> http://lists.infradead.org/pipermail/linux-mediatek/2016-February/003972.html
> >>>>>>
> >>>>>> http://lists.infradead.org/pipermail/linux-mediatek/2016-February/004483.html
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>>>> +};
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_register_device() - register device which needs CMDQ
> >>>>>>>>>> + * @dev:        device
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: cmdq_base pointer or NULL for failed
> >>>>>>>>>> + */
> >>>>>>>>>> +struct cmdq_base *cmdq_register_device(struct device *dev);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_create() - create command queue record
> >>>>>>>>>> + * @dev:        device
> >>>>>>>>>> + * @engine_flag:    command queue engine flag
> >>>>>>>>>> + * @rec_ptr:        command queue record pointer to retrieve
> >>>>>>>>>> cmdq_rec
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_create(struct device *dev, u64 engine_flag,
> >>>>>>>>>> +            struct cmdq_rec **rec_ptr);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_write() - append write command to the command
> >>>>>>>>>> queue record
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + * @value:    the specified target register value
> >>>>>>>>>> + * @base:    the command queue base
> >>>>>>>>>> + * @offset:    register offset from module base
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_write(struct cmdq_rec *rec, u32 value,
> >>>>>>>>>> +           struct cmdq_base *base, u32 offset);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_write_mask() - append write command with mask to
> >>>>>>>>>> the command
> >>>>>>>>>> + *               queue record
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + * @value:    the specified target register value
> >>>>>>>>>> + * @base:    the command queue base
> >>>>>>>>>> + * @offset:    register offset from module base
> >>>>>>>>>> + * @mask:    the specified target register mask
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_write_mask(struct cmdq_rec *rec, u32 value,
> >>>>>>>>>> +            struct cmdq_base *base, u32 offset, u32 mask);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_wfe() - append wait for event command to the
> >>>>>>>>>> command queue reco    rd
> >>>>>>>>>
> >>>>>>>>> reco rd -> record
> >>>>>>>>
> >>>>>>>> Will fix it.
> >>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Matthias
> >>>>>>>>>
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + * @event:    the desired event type to "wait and CLEAR"
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_wfe(struct cmdq_rec *rec, enum cmdq_event event);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_clear_event() - append clear event command to the
> >>>>>>>>>> command queue
> >>>>>>>>>> + *                record
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + * @event:    the desired event to be cleared
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_clear_event(struct cmdq_rec *rec, enum
> >>>>>>>>>> cmdq_event event);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_flush() - trigger CMDQ to execute the recorded
> >>>>>>>>>> commands
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + *
> >>>>>>>>>> + * Trigger CMDQ to execute the recorded commands. Note that
> >>>>>>>>>> this is a
> >>>>>>>>>> + * synchronous flush function. When the function returned,
> >>>>>>>>>> the recorded
> >>>>>>>>>> + * commands have been done.
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_flush(struct cmdq_rec *rec);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_flush_async() - trigger CMDQ to asynchronously
> >>>>>>>>>> execute the recorded
> >>>>>>>>>> + *                commands and call back after ISR is finished
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + * @cb:        called in the end of CMDQ ISR
> >>>>>>>>>> + * @data:    this data will pass back to cb
> >>>>>>>>>> + *
> >>>>>>>>>> + * Return: 0 for success; else the error code is returned
> >>>>>>>>>> + *
> >>>>>>>>>> + * Trigger CMDQ to asynchronously execute the recorded
> >>>>>>>>>> commands and call back
> >>>>>>>>>> + * after ISR is finished. Note that this is an ASYNC
> >>>>>>>>>> function. When the function
> >>>>>>>>>> + * returned, it may or may not be finished. The ISR callback
> >>>>>>>>>> function is called
> >>>>>>>>>> + * in the end of ISR.
> >>>>>>>
> >>>>>>> "The callback is called from the ISR."
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Matthias
> >>>>>>>
> >>>>>>>>>> + */
> >>>>>>>>>> +int cmdq_rec_flush_async(struct cmdq_rec *rec,
> >>>>>>>>>> cmdq_async_flush_cb cb,
> >>>>>>>>>> +             void *data);
> >>>>>>>>>> +
> >>>>>>>>>> +/**
> >>>>>>>>>> + * cmdq_rec_destroy() - destroy command queue record
> >>>>>>>>>> + * @rec:    the command queue record
> >>>>>>>>>> + */
> >>>>>>>>>> +void cmdq_rec_destroy(struct cmdq_rec *rec);
> >>>>>>>>>> +
> >>>>>>>>>> +#endif    /* __MTK_CMDQ_H__ */
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> HS
> >>>>>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>> HS
> >>>>>>
> >>>>
> >>>>
> >>
> >>