Why move all map_sg/unmap_sg for slave channel to its client?

Why move all map_sg/unmap_sg for slave channel to its client?

[viresh kumar]

Hi,

I thought map_sg/unmap_sg for slave channels will be handled according
to the flags passed in prep_slave_sg(). But then i found following patch:

commit 657a77fa7284d8ae28dfa48f1dc5d919bf5b2843
Author: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Date:   Tue Sep 8 17:53:05 2009 -0700

    dmaengine: Move all map_sg/unmap_sg for slave channel to its client
    
    Dan Williams wrote:
    ... DMA-slave clients request specific channels and know the hardware
    details at a low level, so it should not be too high an expectation to
    push dma mapping responsibility to the client.
    
    Also this patch includes DMA_COMPL_{SRC,DEST}_UNMAP_SINGLE support for
    dw_dmac driver.
    
    Acked-by: Maciej Sosnowski <maciej.sosnowski@intel.com>
    Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
    Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
    Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/dma/at_hdmac.c       |   43 +++++++++++++++++++++--------------------
 drivers/dma/dw_dmac.c        |   31 ++++++++++++++++++-----------
 drivers/mmc/host/atmel-mci.c |    9 +++++++-
 3 files changed, 49 insertions(+), 34 deletions(-)


I don't have much knowledge about that discussion, but i think this should be left
configurable. If the client wants to control map/unmap then it can simply pass
DMA_COMPL_SKIP_DEST_UNMAP | DMA_COMPL_SKIP_SRC_UNMAP in flags. I didn't wanted to
skip this in my driver and so i don't pass them.

Why to replicate similar code in client drivers if they need to unmap? What do you say?

-- 
viresh

Re: Why move all map_sg/unmap_sg for slave channel to its client?

[Linus Walleij]

On Thu, Jun 9, 2011 at 8:54 AM, viresh kumar <viresh.kumar@st.com> wrote:

> I thought map_sg/unmap_sg for slave channels will be handled according
> to the flags passed in prep_slave_sg(). But then i found following patch:
> (...)
> I don't have much knowledge about that discussion, but i think this should be left
> configurable.
> If the client wants to control map/unmap then it can simply pass
> DMA_COMPL_SKIP_DEST_UNMAP | DMA_COMPL_SKIP_SRC_UNMAP in flags. I didn't wanted to
> skip this in my driver and so i don't pass them.

What if the same driver is used on many different platforms like say
drivers/tty/serial/amba-pl011.c, and some of the platforms using it
has DMA engines that does not implement mapping/unmapping of
the passed sglist?

In that case I think you have to modify all drivers in drivers/dma/*
to do this mapping, and then you could just make it a required behaviour
and skip the flags altogether.

But apparently that approach was blocked at one point so let's see
what the others say.

Yours,
Linus Walleij

Re: Why move all map_sg/unmap_sg for slave channel to its client?

[Dan Williams]

On Thu, Jun 9, 2011 at 2:38 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 9, 2011 at 8:54 AM, viresh kumar <viresh.kumar@st.com> wrote:
>
>> I thought map_sg/unmap_sg for slave channels will be handled according
>> to the flags passed in prep_slave_sg(). But then i found following patch:
>> (...)
>> I don't have much knowledge about that discussion, but i think this should be left
>> configurable.
>> If the client wants to control map/unmap then it can simply pass
>> DMA_COMPL_SKIP_DEST_UNMAP | DMA_COMPL_SKIP_SRC_UNMAP in flags. I didn't wanted to
>> skip this in my driver and so i don't pass them.
>
> What if the same driver is used on many different platforms like say
> drivers/tty/serial/amba-pl011.c, and some of the platforms using it
> has DMA engines that does not implement mapping/unmapping of
> the passed sglist?
>
> In that case I think you have to modify all drivers in drivers/dma/*
> to do this mapping, and then you could just make it a required behaviour
> and skip the flags altogether.
>
> But apparently that approach was blocked at one point so let's see
> what the others say.

My problem with automatic unmapping support is that the dma-driver
really does not have a chance to get it right except for the trivially
straightforward cases.  One need only look at the current bustage of
raid5 acceleration with respect to overlapping mappings and arm v6.
The dma-driver just knows how to perform "this" operation on "this"
dma address.  It does not know the lifetime of the mapping, or even if
it has the actual dma handle for unmapping versus an offset

For the raid case I've currently convinced myself that the raid client
needs to get directly involved in dma mapping management, rather than
teach all dma drivers a language of how to unmap and when.  Not only
will this fix the overlapping, but it also eliminates the need to map
and remap because the raid client knows the lifetime of  a stripe_head
while the driver only knows the lifetime of a given stripe operation.

For slave-dma maybe there is a lot of common un-mapping logic that can
be reused, but I think that comes from a separate smart library that
understands the dma mapping lifetimes of a given class of clients.
Leave the dma-drivers to just be dumb operators on anonymous dma
addresses.

--
Dan

Re: Why move all map_sg/unmap_sg for slave channel to its client?

[viresh kumar]

On 06/09/2011 11:58 PM, Dan Williams wrote:
> On Thu, Jun 9, 2011 at 2:38 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Jun 9, 2011 at 8:54 AM, viresh kumar <viresh.kumar@st.com> wrote:
>>
>>> I thought map_sg/unmap_sg for slave channels will be handled according
>>> to the flags passed in prep_slave_sg(). But then i found following patch:
>>> (...)
>>> I don't have much knowledge about that discussion, but i think this should be left
>>> configurable.
>>> If the client wants to control map/unmap then it can simply pass
>>> DMA_COMPL_SKIP_DEST_UNMAP | DMA_COMPL_SKIP_SRC_UNMAP in flags. I didn't wanted to
>>> skip this in my driver and so i don't pass them.
>>
>> What if the same driver is used on many different platforms like say
>> drivers/tty/serial/amba-pl011.c, and some of the platforms using it
>> has DMA engines that does not implement mapping/unmapping of
>> the passed sglist?
>>
>> In that case I think you have to modify all drivers in drivers/dma/*
>> to do this mapping, and then you could just make it a required behaviour
>> and skip the flags altogether.
>>
>> But apparently that approach was blocked at one point so let's see
>> what the others say.
> 
> My problem with automatic unmapping support is that the dma-driver
> really does not have a chance to get it right except for the trivially
> straightforward cases.  One need only look at the current bustage of
> raid5 acceleration with respect to overlapping mappings and arm v6.
> The dma-driver just knows how to perform "this" operation on "this"
> dma address.  It does not know the lifetime of the mapping, or even if
> it has the actual dma handle for unmapping versus an offset
> 
> For the raid case I've currently convinced myself that the raid client
> needs to get directly involved in dma mapping management, rather than
> teach all dma drivers a language of how to unmap and when.  Not only
> will this fix the overlapping, but it also eliminates the need to map
> and remap because the raid client knows the lifetime of  a stripe_head
> while the driver only knows the lifetime of a given stripe operation.
> 
> For slave-dma maybe there is a lot of common un-mapping logic that can
> be reused, but I think that comes from a separate smart library that
> understands the dma mapping lifetimes of a given class of clients.
> Leave the dma-drivers to just be dumb operators on anonymous dma
> addresses.
> 

Linus, Dan,

Got it. Thanks for your replies.

-- 
viresh

Re: Why move all map_sg/unmap_sg for slave channel to its client?

[Atsushi Nemoto]

On Fri, 10 Jun 2011 09:11:05 +0530, viresh kumar <viresh.kumar@st.com> wrote:
> >>> I thought map_sg/unmap_sg for slave channels will be handled according
> >>> to the flags passed in prep_slave_sg(). But then i found following patch:
...
> Linus, Dan,
> 
> Got it. Thanks for your replies.

JFYI, the old discussion was here:

https://lkml.org/lkml/2009/7/24/114

---
Atsushi Nemoto