From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laura Abbott Subject: Re: [PATCH] staging: android: ion: add buffer flag update ioctl Date: Wed, 2 Jan 2019 14:31:06 -0800 Message-ID: <58278bab-413c-ef5e-3074-5afbcc1b86de@redhat.com> References: <1545239943-15414-1-git-send-email-prime.zeng@hisilicon.com> <678F3D1BB717D949B966B68EAEB446ED24E2926A@dggemm526-mbx.china.huawei.com> <786ad55f-4651-56ce-cd5c-ca02f7ac4093@redhat.com> <678F3D1BB717D949B966B68EAEB446ED24E2B495@dggemm526-mbx.china.huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <678F3D1BB717D949B966B68EAEB446ED24E2B495@dggemm526-mbx.china.huawei.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: "Zengtao (B)" , "sumit.semwal@linaro.org" Cc: Greg Kroah-Hartman , =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Todd Kjos , Martijn Coenen , Joel Fernandes , "devel@driverdev.osuosl.org" , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , "linux-kernel@vger.kernel.org" List-Id: dri-devel@lists.freedesktop.org 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) ; sumit.semwal@linaro.org >> Cc: Greg Kroah-Hartman ; Arve Hjønnevåg >> ; Todd Kjos ; Martijn Coenen >> ; Joel Fernandes ; >> 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) ; >> sumit.semwal@linaro.org >>>> Cc: Greg Kroah-Hartman ; Arve >> Hjønnevåg >>>> ; Todd Kjos ; Martijn >> Coenen >>>> ; Joel Fernandes ; >>>> 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/2018-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. Thanks, Laura >> >>>> >>>>> Signed-off-by: Zeng Tao >>>>> --- >>>>> 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 >>>>> >>> >