Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

On 05/08/13 14:26, Archit Taneja wrote:
> On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:

>>> +/*
>>> + * submit a list of DMA descriptors to the VPE VPDMA, do not wait
>>> for completion
>>> + */
>>> +int vpdma_submit_descs(struct vpdma_data *vpdma, struct
>>> vpdma_desc_list *list)
>>> +{
>>> +    /* we always use the first list */
>>> +    int list_num = 0;
>>> +    int list_size;
>>> +
>>> +    if (vpdma_list_busy(vpdma, list_num))
>>> +        return -EBUSY;
>>> +
>>> +    /* 16-byte granularity */
>>> +    list_size = (list->next - list->buf.addr) >> 4;
>>> +
>>> +    write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list->buf.dma_addr);
>>> +    wmb();
>>
>> What is the wmb() for?
> 
> VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise
> VPDMA doesn't work. wmb() ensures the ordering.

Are you sure it's needed? Here's an interesting thread about writing and
reading to registers: http://marc.info/?t=130588594900002&r=1&w=2

>>> +
>>> +    for (i = 0; i < 100; i++) {        /* max 1 second */
>>> +        msleep_interruptible(10);
>>
>> You call interruptible version here, but you don't handle the
>> interrupted case. I believe the loop will just continue looping, even if
>> the user interrupted.
> 
> Okay. I think I don't understand the interruptible version correctly. We
> don't need to msleep_interruptible here, we aren't waiting on any wake
> up event, we just want to wait till a bit gets set.

Well, I think the interruptible versions should be used when the user
(wel, userspace program) initiates the action. The user should have the
option to interrupt a possibly long running operation, which is what
msleep_interruptible() makes possible.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]