Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 27 July 2011 14:32, Koul, Vinod <vinod.koul@xxxxxxxxx> wrote:
>> >> >>> Correct, it is meant that chan_id is only a sysfs property. ÂAny
>> >> >>> driver usage that is assuming chan_id is anything more than a
>> >> >>> guaranteed unique number within a given dma_device's list of channels
>> >> >>> is probably inferring too much.
>> >> >>
>> >> >> So you mean dmac/client drivers are wrong if they make use of chan_id.
>> >> >> They shouldn't count upon it's value - which is set by DMA API for a completely
>> >> >> independent purpose, i.e, creating contiguous sysfs entries.
>> >> >
>> >> > They can count on it being unique, and maybe the fact that it is in
>> >> > the same order as dma_device.channels.
>> >> The latter implies the former. And it is already the dmac driver that
>> >> decides the
>> >> rank of a channel in the list.
>> >>
>> >> >
>> >> >>
>> >> >> Since "chan_id is only a sysfs property" and the fact that it is used
>> >> >> only _once_
>> >> >> by the DMA API
>> >> >>
>> >> >> In drivers/dma/dmaengine.c
>> >> >>
>> >> >> Â Â Âchan->chan_id = chancnt++;
>> >> >> Â Â Âdev_set_name(&chan->dev->device, "dma%dchan%d",
>> >> >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â device->dev_id, chan->chan_id);
>> >> >>
>> >> >>
>> >> >> Can't we do away with chan_id altogether ? by having
>> >> >>
>> >> >> Â Â Âdev_set_name(&chan->dev->device, "dma%dchan%d",
>> >> >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â device->dev_id, chancnt++);
>> >> >>
>> >> >> I mean why make every instance of dma_chan bigger by 4bytes ?
>> >> >>
>> >> >> So why shouldn't we remove chan_id completely from the DMA API ?
>> >> >
>> >> > Good point... ÂLet's remove chan_id from the core and push it into the
>> >> > drivers that need it.
>> >> >
>> >> If you agree, I would preserve the chan_id in 'struct dma_chan' but remove
>> >> any assignment to it in dmaengine.c Âand let the dmac drivers use it freely.
>> >> That would:-
>> >> a) Let dmac drivers decide what numbers they want to show up in sysfs.
>> >> b) chan_id is easily reachable by client drivers, so it is better this way.
>> >> c) It would mean lesser and simpler changes to extant users of it.
>> > But this can cause conflict between two controllers who think they are
>> > assigning unique numbers.
>> Could you please clarify, which two controllers ?
> You can have two different DMACs in same system. At least I have two
> from current intel_mid_dma which are used. Both give their channel id
> starting from 0, 1....
> Further as we integrate video, audio, spi, emmc dmacs possibility of
> having multiple dmacs will increase in a system
Most of Samsung's S5P series have 3 DMACs - 2 for peripherals and 1 for mem->mem
But that is not the point.

This patch in no way affects what values currently a dmac driver
assigns to chan_id

>>
>> > IMO sysfs representation needs to be with
>> > dmaengine only. How do we guarantee uniqueness b/w two controllers?
>> Again, how is chan_id currently unique between two controllers ?
>>
>> Btw, do you not want to keep chan_id in dma_chan or do you not want to change
>> anything at all ?
>>
>> >
>> > Also I am not sure about the approach: The whole point is to make filter
>> > function select based on some channel number "x", but you should filter
>> > channels based on controller you want and capability of a channel. If
>> > caps is not enough to filter we should add more flags but asking that I
>> > need channel 'y' doesn't sound right to me.
>> > This is all coming from that fact that some drivers assume channel 'a'
>> > is for this type of transfer and channel 'b' for something else, for a
>> > dma controller that really doesn't matter, as all channels have similar
>> > capabilities and can do similar things so you should
>> > _get_channel_based_on_caps rather than on some numbering.
>> >
>> > Lastly, why do you need a channel reserved for some type of transfer, is
>> > it for assigning h/w interface for dma transfer, if so that can be
>> > achieved in different ways as well
>>
>> Please look at this patch from POV of utility of chan_id, which isn't much
>> currently as explainined.
> Sorry I didn't get you.
> As I understand you are trying to simplify the filter function by
> assigning unique ids to all channels,
No dear. Let me put it precisely.

Even if we make no further change to the dmaengine, this patch is the right
thing to do today.

Please have a look at the patch and figure if it breaks anything or is not the
right change. Plz ignore what you _think_ I plan to do next.

> but whole point itself is not
> quite right, as I said selection should be based on capability and not
> channel number "x".
* I absolutely agree *


> Can you please help me understand why channel number "x" is important
> and strictly needed by some client?
I never said that. I don't think so.

I request you to please have a look at

1) What I propose
        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html

2) Why RMK thinks I am the biggest idiot on earth
        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html

3) How I ask for better proof of that
       http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059223.html

On a serious note, my proposal, and the reply, shows the possibility
of having :-
a) Client drivers that are truly platform agnostic -- no platform_data
poking for
      channel selection
b) A standard way to express capabilities of a channel -- chan_id is just one,
     we can employ different method if need be.
c) DMAENGINE API smart enough to provide best channel selection for a request.

Finally, let us please take this discussion there, because this patch should be
judged only under it's own light.

Thanks,
Jassi


-- 
Linaro.org â Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106 Â-
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/