On Mon, Aug 04, 2014 at 06:30:51PM +0200, Laurent Pinchart wrote:
> > > +static int rcar_dmac_alloc_chan_resources(struct dma_chan *chan)
> > > +{
> > > + struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
> > > + int ret;
> > > +
> > > + INIT_LIST_HEAD(&rchan->desc.free);
> > > + INIT_LIST_HEAD(&rchan->desc.pending);
> > > + INIT_LIST_HEAD(&rchan->desc.active);
> > > + INIT_LIST_HEAD(&rchan->desc.done);
> > > + INIT_LIST_HEAD(&rchan->desc.wait);
> > > + INIT_LIST_HEAD(&rchan->desc.hw_free);
> > > + INIT_LIST_HEAD(&rchan->desc.pages);
> >
> > Seriously we need so many lists? 1st three makes sense but what is delta b/w
> > done and free. Once a descriptor is done it should be moved to freee list.
> >
> > What does wait list mean and the last two?
>
> The free list contains descriptors that have been allocated and are not used.
> Those can be descriptors preallocated and not used yet, or descriptors that
> have been used and recycled.
>
> The pending list contains descriptors submitted (through dmaengine_submit) but
> not issued (with dma_async_issue_pending) yet.
>
> The active list contains descriptors issued with dma_async_issue_pending. The
> list tail is or will be processed by the hardware, and the other elements in
> the list will be processed in turn.
>
> The done list contains descriptors that have been processed by the hardware
> but haven't been processed by the interrupt thread (I'll discuss thread
> vs.tasklet below) yet. Descriptors are added to the done list in the interrupt
> handler, and taken from the list by the interrupt thread.
>
> The wait list contains descriptors processed by the interrupt thread for which
> the completion callback has been called, but that haven't been acked (tested
> with async_tx_test_ack) yet. Descriptors are added to that list after calling
> their callback function, and the list is then processed by
> rcar_dmac_desc_recycle_acked (called both from the interrupt thread and from
> the descriptor allocation function) to move all acked descriptors back to the
> free list.
Okay but do we really need the two list for transistions? The descriptors in
active_list can be marked for intermediate stages and then pushed to
free_list.
Also second question, do you submit multiple clinet request in a single go
to hardware or it is usually one descriptor?
> > okay this bit needs modification. The channel can be configured in any
> > direction. SO you can have one descriptor doing DMA_DEV_TO_MEM and another
> > DMA_MEM_TO_DEV. The prep_ calls have direction arg to be used, so please
> > store both :)
>
> Right, but before fixing that, let's document exactly how the struct
> dma_slave_config field are supposed to be used.
>
> The direction field is documented in the header as
>
> * @direction: whether the data shall go in or out on this slave
> * channel, right now. DMA_MEM_TO_DEV and DMA_DEV_TO_MEM are
> * legal values.
>
> That seems to contradict your statement.
Yes certainly, we need to fix that too :)
> > > +static size_t rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
> > > + dma_cookie_t cookie)
> > > +{
> > > + struct rcar_dmac_desc *desc = chan->desc.running;
> > > + struct rcar_dmac_hw_desc *hwdesc;
> > > + size_t residue = 0;
> > > +
> > > + if (!desc)
> > > + return 0;
> >
> > Why?
> > We should be able to query even when channel is not running, right?
>
> Good question, should we ? :-)
>
> This would require going through all lists to find the descriptor
> corresponding to the cookie, and returning either 0 (if the descriptor has
> been processed completely) or the descriptor length (if it hasn't been
> processed yet). This is potentially costly.
Not really, the status of descriptor will tell you.
> The first case can be easily handled using the cookie only, when
> dma_cookie_status() state returns DMA_COMPLETE then we know that the residue
> is 0. This is actually the current behaviour, as dma_cookie_status() zeroes
> the residue field.a
yes this is the key
>
> Is the second case something we need to support ? chan->desc.running is only
> NULL when there's no descriptor to process. In that case the cookie can either
> correspond to a descriptor already complete (first case), a descriptor
> prepared but not submitted or an invalid descriptor (in which case we can't
> report the residue anyway). Is there really a use case for reporting the
> residue for a descriptor not submitted ? That seems like a bad use of the API
> to me, I think it would be better to forbid it.
So you need to check only running list. For the queued up descriptor it is
easy enough. For the one which is running you are already doing the
calculation. For completed but still not preocessed it is zero anyway
> > > +static int rcar_dmac_slave_caps(struct dma_chan *chan,
> > > + struct dma_slave_caps *caps)
> > > +{
> > > + memset(caps, 0, sizeof(*caps));
> > > +
> > > + /*
> > > + * The device supports all widths from 1 to 64 bytes. As the
> > > + * dma_slave_buswidth enumeration is limited to 8 bytes, set the
> > > + * numerical value directly.
> > > + */
> > > + caps->src_addr_widths = 0x7f;
> > > + caps->dstn_addr_widths = 0x7f;
> >
> > no magic numbers pls
>
> Come on, 0x7f is hardly magic ;-)
For you yes, but down the line someone would scrath their head on why this
was 0x7f :)
>
> Should I add DMA_SLAVE_BUSWIDTH_16_BYTES, DMA_SLAVE_BUSWIDTH_32_BYTES and
> DMA_SLAVE_BUSWIDTH_64_BYTES to enum dma_slave_buswidth ?
Yup
> > > +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
> > > +static int rcar_dmac_suspend(struct device *dev)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int rcar_dmac_resume(struct device *dev)
> > > +{
> > > + struct rcar_dmac *dmac = dev_get_drvdata(dev);
> > > +
> > > + return rcar_dmac_init(dmac);
> > > +}
> > > +#endif
> >
> > I dont think you need these as you are using PM macros.
>
> Then gcc will complain above functions being defined and not used.
oh, then we should fix in the macro itself rather than adding in driver.
> > > +static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
> > > + struct rcar_dmac_chan *rchan,
> > > + unsigned int index)
> > > +{
> > > + struct platform_device *pdev = to_platform_device(dmac->dev);
> > > + struct dma_chan *chan = &rchan->chan;
> > > + char irqname[5];
> > > + int irq;
> > > + int ret;
> > > +
> > > + rchan->index = index;
> > > + rchan->iomem = dmac->iomem + RCAR_DMAC_CHAN_OFFSET(index);
> > > + rchan->mid_rid = -EINVAL;
> > > +
> > > + spin_lock_init(&rchan->lock);
> > > + mutex_init(&rchan->power_lock);
> > > +
> > > + /* Request the channel interrupt. */
> > > + sprintf(irqname, "ch%u", index);
> > > + irq = platform_get_irq_byname(pdev, irqname);
> > > + if (irq < 0) {
> > > + dev_err(dmac->dev, "no IRQ specified for channel %u\n", index);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + rchan->irqname = devm_kmalloc(dmac->dev,
> > > + strlen(dev_name(dmac->dev)) + 4,
> > > + GFP_KERNEL);
> > > + if (!rchan->irqname)
> > > + return -ENOMEM;
> > > + sprintf(rchan->irqname, "%s:%u", dev_name(dmac->dev), index);
> > > +
> > > + ret = devm_request_threaded_irq(dmac->dev, irq,
> rcar_dmac_isr_channel,
> > > + rcar_dmac_isr_channel_thread, 0,
> > > + rchan->irqname, rchan);
> >
> > I know sh drivers do this but WHY. All other dmac drivers are happy with API
> > and do tasklets.
>
> Honestly, I don't know. I've just copied that code from shdma-base.c. It was
> introduced in commit 9a7b8e002e331d0599127f16613c32f425a14f2c ("dmaengine: add
> an shdma-base library") without a specific explanation.
>
> I can change that. What's the rationale though ? Is it just a matter of
> realtime constraints, or are there other reasons for using tasklets instead of
> threaded IRQs ?
>
> By the way, as callbacks can prepare new descriptors, running them in a
> tasklet means I can't sleep in the prep_* functions. It seems I can't sleep
> there for other reasons anyway, but I need to fix it, and it leaves me with no
> way to properly implement runtime PM support without using a work queue. I've
> raised the issue in "DMA engine API issue" mail thread, I'd like to sort it
> out first before fixing the driver.
Okay lets sort that out first
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
