Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] staging: ion: Rework ion_map_dma_buf() to minimize
 re-mapping
To:     John Stultz <john.stultz@linaro.org>
Cc:     lkml <linux-kernel@vger.kernel.org>,
        Beata Michalska <Beata.Michalska@arm.com>,
        Matt Szczesiak <matt.szczesiak@arm.com>,
        Anders Pedersen <Anders.Pedersen@arm.com>,
        John Reitan <John.Reitan@arm.com>,
        Liam Mark <lmark@codeaurora.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        dri-devel <dri-devel@lists.freedesktop.org>
References: <1539214413-26173-1-git-send-email-john.stultz@linaro.org>
 <7534ca1d-f874-7809-6125-d9fc72f70e39@redhat.com>
 <CALAqxLUd-L+dj=GD5F=PkP5o-FokMCeYHWRJZs24R9-Grbf=PA@mail.gmail.com>
From:   Laura Abbott <labbott@redhat.com>
Message-ID: <e936704c-9bb5-8da8-3d30-e7c9a12a3b07@redhat.com>
Date:   Sun, 21 Oct 2018 02:13:39 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <CALAqxLUd-L+dj=GD5F=PkP5o-FokMCeYHWRJZs24R9-Grbf=PA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 10/17/2018 07:53 PM, John Stultz wrote:
> On Fri, Oct 12, 2018 at 10:51 AM, Laura Abbott <labbott@redhat.com> wrote:
>>
>> I suspect most of the cost of the dma_map/dma_unmap is from the
>> cache flushing and not the actual mapping operations. If this
>> is the case, another option might be to figure out how to
>> incorporate dma_attrs so drivers can use DMA_ATTR_SKIP_CPU_SYNC
>> to decide when they actually want to sync.
> 
> So just to confirm on this point, I basically tested the following
> change (whitespace corrupt, sorry):
> 
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 24cb666..e76b0e2 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -273,8 +273,8 @@ static struct sg_table *ion_map_dma_buf(struct
> dma_buf_attachment *attachment,
> 
>          table = a->table;
> 
> -       if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> -                       direction))
> +       if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> +                             direction,  DMA_ATTR_SKIP_CPU_SYNC))
>                  return ERR_PTR(-ENOMEM);
> 
>          return table;
> @@ -284,7 +284,7 @@ static void ion_unmap_dma_buf(struct
> dma_buf_attachment *attachment,
>                                struct sg_table *table,
>                                enum dma_data_direction direction)
>   {
> -       dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> +       dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents,
> direction, DMA_ATTR_SKIP_CPU_SYNC);
>   }
> 
>   static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> 
> 
> And indeed, that performed similarly to the pre 4.12 ION code (and it
> also had some of the same image caching error garbage we've seen w/
> 4.9 era kernels, which my earlier patch didn't have).
> 
> So yes, it seems having some way to conditionally skip cpu sync would
> be good.  Though I'm not sure what sort of interface to using this you
> might have in mind?
> 

I hadn't quite gotten anything fully formed but I think solving this
will require changes at the dma-buf layer. One idea I had was allowing
dma_attrs to be set per attachment, the other was extending the
dma_buf_map_attachment to take an attrs argument. I'm at OSSEU this
week but I didn't want to forget about this. I'll see if I can turn
this into a more coherent proposal unless you get to it first.

Thanks,
Laura

> thanks
> -john
>