From: Laura Abbott <labbott@redhat.com>
To: "Zengtao (B)" <prime.zeng@hisilicon.com>,
"sumit.semwal@linaro.org" <sumit.semwal@linaro.org>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Arve Hjønnevåg" <arve@android.com>,
"Todd Kjos" <tkjos@android.com>,
"Martijn Coenen" <maco@android.com>,
"Joel Fernandes" <joel@joelfernandes.org>,
"devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
"linaro-mm-sig@lists.linaro.org" <linaro-mm-sig@lists.linaro.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl
Date: Thu, 3 Jan 2019 17:52:57 -0800 [thread overview]
Message-ID: <e547f9ce-64ff-74e5-e0c8-dbf3f013fe74@redhat.com> (raw)
In-Reply-To: <678F3D1BB717D949B966B68EAEB446ED24E31270@dggemm526-mbx.china.huawei.com>
On 1/3/19 5:42 PM, Zengtao (B) wrote:
>> -----Original Message-----
>> From: Laura Abbott [mailto:labbott@redhat.com]
>> Sent: Thursday, January 03, 2019 6:31 AM
>> To: Zengtao (B) <prime.zeng@hisilicon.com>; sumit.semwal@linaro.org
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arve Hjønnevåg
>> <arve@android.com>; Todd Kjos <tkjos@android.com>; Martijn Coenen
>> <maco@android.com>; Joel Fernandes <joel@joelfernandes.org>;
>> devel@driverdev.osuosl.org; dri-devel@lists.freedesktop.org;
>> linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl
>>
>> On 12/23/18 6:47 PM, Zengtao (B) wrote:
>>> Hi laura:
>>>
>>>> -----Original Message-----
>>>> From: Laura Abbott [mailto:labbott@redhat.com]
>>>> Sent: Friday, December 21, 2018 4:50 AM
>>>> To: Zengtao (B) <prime.zeng@hisilicon.com>;
>> sumit.semwal@linaro.org
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arve
>> Hjønnevåg
>>>> <arve@android.com>; Todd Kjos <tkjos@android.com>; Martijn
>> Coenen
>>>> <maco@android.com>; Joel Fernandes <joel@joelfernandes.org>;
>>>> devel@driverdev.osuosl.org; dri-devel@lists.freedesktop.org;
>>>> linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH] staging: android: ion: add buffer flag update
>>>> ioctl
>>>>
>>>> On 12/19/18 5:39 PM, Zengtao (B) wrote:
>>>>> Hi laura:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Laura Abbott [mailto:labbott@redhat.com]
>>>>>> Sent: Thursday, December 20, 2018 2:10 AM
>>>>>> To: Zengtao (B) <prime.zeng@hisilicon.com>;
>>>> sumit.semwal@linaro.org
>>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arve
>>>> Hjønnevåg
>>>>>> <arve@android.com>; Todd Kjos <tkjos@android.com>; Martijn
>>>> Coenen
>>>>>> <maco@android.com>; Joel Fernandes <joel@joelfernandes.org>;
>>>>>> devel@driverdev.osuosl.org; dri-devel@lists.freedesktop.org;
>>>>>> linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org
>>>>>> Subject: Re: [PATCH] staging: android: ion: add buffer flag update
>>>>>> ioctl
>>>>>>
>>>>>> On 12/19/18 9:19 AM, Zeng Tao wrote:
>>>>>>> In some usecases, the buffer cached attribute is not determined at
>>>>>>> allocation time, it's determined just before the real cpu mapping.
>>>>>>> And from the memory view of point, a buffer should not have the
>>>>>> cached
>>>>>>> attribute util is really mapped by the cpu. So in this patch, we
>>>>>>> introduced the new ioctl command to target the requirement.
>>>>>>>
>>>>>>
>>>>>> This is racy and error prone. Can you explain more what problem
>> you
>>>>>> are trying to solve?
>>>>>
>>>>> My use case is like this:
>>>>> 1. There are two process A and B, A takes case of ion buffer
>>>>> allocation,
>>>> and
>>>>> pass the buffer fd to B, then B maps and uses it.
>>>>> 2. Process B need to map the buffer with different cached attribute
>>>>> for different use case, for example, if the buffer is used for pure
>>>>> software algorithm, then we need to map it as cached, otherwise
>>>>> non-cached, and B needs to deal with both cases.
>>>>> And unfortunately the mmap syscall takes no cached flags and we
>>>>> can't decide the cache attribute when we are doing the mmap, so I
>>>>> introduce new the ioctl even though I think the solution is not as
>> good.
>>>>>
>>>>>
>>>>
>>>> Thanks for the explanation, this was about the use case I expected.
>>>> I'm pretty sure I had this exact problem once upon a time and we
>>>> didn't come up with a solution. I'd still like to get rid of uncached
>>>> buffers in general and just use cached buffers (see
>>>> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/201
>>>> 8-N
>>>> ovember/128842.html)
>>>> What's your usecase for uncached buffers?
>>>
>>> Some buffers are only used by HW, in this case, we tend to use
>> uncached buffers.
>>> But sometimes the SW need to read few buffer contents for debug
>>> purpose and no synchronization is needed.
>>>
>>
>> If this is primarily for debug, that's not really a compelling reason to
>> support uncached buffers. It's incredibly difficult to do uncached buffers
>> correctly I'd rather spend effort on making the cached use cases work
>> better.
>>
> No, not only for debug, I meant if the buffers are uncached, the SW will only mmap
> them for debug or even don't need to mmap them.
> So for the two kinds of buffers:
> 1. uncached: only the HW need to access it, the SW don't need to mmap it or just for debug.
> 2. cached: both the HW and the SW need to access it.
> The ION is designed as a generalized memory manager, I think we should cover as many
> requirements as we can in the ION design, so I 'd rather that we keep the uncached support.
> And I don’t quite understand the difficulty you mentioned to support the uncached buffers, would
> you explain a little more about it.
>
We end up with aliasing problems. Each kernel page still has a cached
mapping so it's very difficult to keep the two mappings in sync.
https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@DESKTOP-E1NTVVP.localdomain/
This thread does a better job of explaining the problem and the
objections to uncached buffers.
And if the software never touches the buffer, why does it
matter if the buffer is uncached?
Laura
> Thanks
> Zengtao
>
>>
>>>>
>>>>>>
>>>>>>> Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
>>>>>>> ---
>>>>>>> drivers/staging/android/ion/ion-ioctl.c | 4 ++++
>>>>>>> drivers/staging/android/ion/ion.c | 17
>>>> +++++++++++++++++
>>>>>>> drivers/staging/android/ion/ion.h | 1 +
>>>>>>> drivers/staging/android/uapi/ion.h | 22
>>>>>> ++++++++++++++++++++++
>>>>>>> 4 files changed, 44 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>>>>>> b/drivers/staging/android/ion/ion-ioctl.c
>>>>>>> index a8d3cc4..60bb702 100644
>>>>>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>>>>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>>>>>> @@ -12,6 +12,7 @@
>>>>>>>
>>>>>>> union ion_ioctl_arg {
>>>>>>> struct ion_allocation_data allocation;
>>>>>>> + struct ion_buffer_flag_data update;
>>>>>>> struct ion_heap_query query;
>>>>>>> };
>>>>>>>
>>>>>>> @@ -83,6 +84,9 @@ long ion_ioctl(struct file *filp, unsigned int
>>>>>>> cmd, unsigned long arg)
>>>>>>>
>>>>>>> break;
>>>>>>> }
>>>>>>> + case ION_IOC_BUFFER_UPDATE:
>>>>>>> + ret = ion_buffer_update(data.update.fd, data.update.flags);
>>>>>>> + break;
>>>>>>> case ION_IOC_HEAP_QUERY:
>>>>>>> ret = ion_query_heaps(&data.query);
>>>>>>> break;
>>>>>>> diff --git a/drivers/staging/android/ion/ion.c
>>>>>>> b/drivers/staging/android/ion/ion.c
>>>>>>> index 9907332..f1404dc 100644
>>>>>>> --- a/drivers/staging/android/ion/ion.c
>>>>>>> +++ b/drivers/staging/android/ion/ion.c
>>>>>>> @@ -436,6 +436,23 @@ int ion_alloc(size_t len, unsigned int
>>>>>> heap_id_mask, unsigned int flags)
>>>>>>> return fd;
>>>>>>> }
>>>>>>>
>>>>>>> +int ion_buffer_update(unsigned int fd, unsigned int flags) {
>>>>>>> + struct dma_buf *dmabuf;
>>>>>>> + struct ion_buffer *buffer;
>>>>>>> +
>>>>>>> + dmabuf = dma_buf_get(fd);
>>>>>>> +
>>>>>>> + if (!dmabuf)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + buffer = dmabuf->priv;
>>>>>>> + buffer->flags = flags;
>>>>>>> + dma_buf_put(dmabuf);
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> int ion_query_heaps(struct ion_heap_query *query)
>>>>>>> {
>>>>>>> struct ion_device *dev = internal_dev; diff --git
>>>>>>> a/drivers/staging/android/ion/ion.h
>>>>>>> b/drivers/staging/android/ion/ion.h
>>>>>>> index c006fc1..99bf9ab 100644
>>>>>>> --- a/drivers/staging/android/ion/ion.h
>>>>>>> +++ b/drivers/staging/android/ion/ion.h
>>>>>>> @@ -199,6 +199,7 @@ int ion_heap_pages_zero(struct page
>> *page,
>>>>>> size_t size, pgprot_t pgprot);
>>>>>>> int ion_alloc(size_t len,
>>>>>>> unsigned int heap_id_mask,
>>>>>>> unsigned int flags);
>>>>>>> +int ion_buffer_update(unsigned int fd, unsigned int flags);
>>>>>>>
>>>>>>> /**
>>>>>>> * ion_heap_init_shrinker
>>>>>>> diff --git a/drivers/staging/android/uapi/ion.h
>>>>>>> b/drivers/staging/android/uapi/ion.h
>>>>>>> index 5d70098..99753fc 100644
>>>>>>> --- a/drivers/staging/android/uapi/ion.h
>>>>>>> +++ b/drivers/staging/android/uapi/ion.h
>>>>>>> @@ -74,6 +74,20 @@ struct ion_allocation_data {
>>>>>>> __u32 unused;
>>>>>>> };
>>>>>>>
>>>>>>> +/**
>>>>>>> + * struct ion_buffer_flag_data - metadata passed from userspace
>>>>>>> +for update
>>>>>>> + * buffer flags
>>>>>>> + * @fd: file descriptor of the buffer
>>>>>>> + * @flags: flags passed to the buffer
>>>>>>> + *
>>>>>>> + * Provided by userspace as an argument to the ioctl */
>>>>>>> +
>>>>>>> +struct ion_buffer_flag_data {
>>>>>>> + __u32 fd;
>>>>>>> + __u32 flags;
>>>>>>> +}
>>>>>>> +
>>>>>>> #define MAX_HEAP_NAME 32
>>>>>>>
>>>>>>> /**
>>>>>>> @@ -116,6 +130,14 @@ struct ion_heap_query {
>>>>>>> struct ion_allocation_data)
>>>>>>>
>>>>>>> /**
>>>>>>> + * DOC: ION_IOC_BUFFER_UPDATE - update the specified ion
>> buffer
>>>>>> flags
>>>>>>> + *
>>>>>>> + * Takes an ion_buffer_flag_data structure and returns the result
>>>>>>> +of the
>>>>>>> + * buffer flag update operation.
>>>>>>> + */
>>>>>>> +#define ION_IOC_BUFFER_UPDATE _IOWR(ION_IOC_MAGIC, 1, \
>>>>>>> + struct ion_buffer_flag_data)
>>>>>>> +/**
>>>>>>> * DOC: ION_IOC_HEAP_QUERY - information about available
>>>> heaps
>>>>>>> *
>>>>>>> * Takes an ion_heap_query structure and populates
>> information
>>>>>> about
>>>>>>>
>>>>>
>>>
>
next prev parent reply other threads:[~2019-01-04 1:53 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-19 17:19 [PATCH] staging: android: ion: add buffer flag update ioctl Zeng Tao
2018-12-19 15:41 ` kbuild test robot
2018-12-19 18:09 ` Laura Abbott
2018-12-20 1:39 ` Zengtao (B)
2018-12-20 20:49 ` Laura Abbott
2018-12-24 2:47 ` Zengtao (B)
2019-01-02 22:31 ` Laura Abbott
2019-01-04 1:42 ` Zengtao (B)
2019-01-04 1:52 ` Laura Abbott [this message]
2019-01-15 3:04 ` Zengtao (B)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e547f9ce-64ff-74e5-e0c8-dbf3f013fe74@redhat.com \
--to=labbott@redhat.com \
--cc=arve@android.com \
--cc=devel@driverdev.osuosl.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=joel@joelfernandes.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maco@android.com \
--cc=prime.zeng@hisilicon.com \
--cc=sumit.semwal@linaro.org \
--cc=tkjos@android.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).