Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion
On Tue, Oct 13 2015 at 11:14:23 AM, Andrew wrote:
> On 2015-10-12 21:39, Mitchel Humpherys wrote:
>> On Tue, Oct 06 2015 at 05:35:41 PM, Rob Herring
>> wrote:
>>> On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott
>>> wrote:
>>
>> [...]
>>
>>>> +Example:
>>>> +
>>>> + ion {
>>>> + compatbile = "linux,ion";
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> +
>>>> + ion-system-heap {
>>>> + linux,ion-heap-id = <0>;
>>>> + linux,ion-heap-type = ;
>>>> + linux,ion-heap-name = "system";
>>>
>>> How does this vary across platforms? Is all of this being pushed down
>>> to DT, because there is no coordination of this at the kernel ABI
>>> level across platforms. In other words, why can't heap 0 be hardcoded
>>> as system heap in the driver. It seems to me any 1 of these 3
>>> properties could be used to derive the other 2.
>>
>> The heap-id<->heap-type mapping isn't necessarily 1:1. As Laura
>> indicated elsewhere on this thread, a given heap might need to be
>> contiguous on one platform but not on another. In that case you just
>> swap out the heap-type here and there's no need for userspace to change.
>>
>> The heap-name, OTOH, could be derived from the heap-id, which is what we
>> hackishly do here [1] and here[2].
>
> By the way, since we agreed that heap id and heap type mappings
> are not 1:1 - we have a problem with the current API.
>
> In userspace we currently have this:
>
> int ion_alloc(int fd, size_t len, size_t align, unsigned int heap_mask,
> unsigned int flags, ion_user_handle_t *handle);
>
> We do not specify here what TYPE of heap we want the allocation to come
> from.
> This may lead to very unpleasant stuff when porting from one platfrom to
> another.
What "unpleasant stuff" are you referring to, exactly? Abstracting the
heap type away from userspace has actually made our lives easier since
userspace doesn't need to know anything about the properties of the
underlying platform. It just asks for a buffer from the "camera" heap,
for example. On some platforms that's contiguous, on others it's not.
-Mitch
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion
On Tue, Oct 06 2015 at 05:35:41 PM, Rob Herring wrote:
> On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott
> wrote:
[...]
>> +Example:
>> +
>> + ion {
>> + compatbile = "linux,ion";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + ion-system-heap {
>> + linux,ion-heap-id = <0>;
>> + linux,ion-heap-type = ;
>> + linux,ion-heap-name = "system";
>
> How does this vary across platforms? Is all of this being pushed down
> to DT, because there is no coordination of this at the kernel ABI
> level across platforms. In other words, why can't heap 0 be hardcoded
> as system heap in the driver. It seems to me any 1 of these 3
> properties could be used to derive the other 2.
The heap-id<->heap-type mapping isn't necessarily 1:1. As Laura
indicated elsewhere on this thread, a given heap might need to be
contiguous on one platform but not on another. In that case you just
swap out the heap-type here and there's no need for userspace to change.
The heap-name, OTOH, could be derived from the heap-id, which is what we
hackishly do here [1] and here[2].
[1]
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.14/tree/drivers/staging/android/ion/msm/msm_ion.c?h=msm-3.14#n53
[2]
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.14/tree/drivers/staging/android/ion/msm/msm_ion.c?h=msm-3.14#n398
-Mitch
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: ion: chunk_heap: use %pad for printing dma_addr_t's
On Mon, May 04 2015 at 01:05:50 PM, Colin Cross wrote:
> On Mon, May 4, 2015 at 1:22 AM, Dan Carpenter
> wrote:
>> On Thu, Apr 09, 2015 at 06:10:04PM -0700, Mitchel Humpherys wrote:
>>> We're currently using %lu and %ld to print some variables of type
>>> dma_addr_t, which results in the following warning when dma_addr_t is
>>> 64-bits wide:
>>>
>>> drivers/staging/android/ion/ion_chunk_heap.c: In function
>>> 'ion_chunk_heap_create':
>>> drivers/staging/android/ion/ion_chunk_heap.c:176:2: warning: format
>>> '%lu' expects argument of type 'long unsigned int', but argument 3 has type
>>> 'dma_addr_t' [-Wformat=]
>>> pr_info("%s: base %lu size %zu align %ld\n", __func__,
>>> chunk_heap->base,
>>> ^
>>> drivers/staging/android/ion/ion_chunk_heap.c:176:2: warning: format
>>> '%ld' expects argument of type 'long int', but argument 5 has type
>>> 'dma_addr_t' [-Wformat=]
>>>
>>> Fix this by using %pad as instructed in printk-formats.txt.
>>>
>>> Signed-off-by: Mitchel Humpherys
>>
>> This one was just merged and I was about to email you that it introduces
>> some new Smatch warnings, but actually looking at it, it's just wrong.
>>
>> We want to print "chunk_heap->base" and not "&chunk_heap->base".
>
> This would be correct if base was a dma_addr_t...
>
>> And anyway "&chunk_heap->base" is a regular pointer, not a dma_addr_t.
>
> But it is actually an ion_phys_addr_t, which is currently typedef'd to
> unsigned long. Are you using a local patch that replaces
> ion_phys_addr_t with dma_addr_t?
>
>> So please send a new patch that removes the &.
>
> Removing the & is not correct, lib/vsprintf.c will dereference the arg
> for %pad or %pap. I think this patch should just be dropped, the old
> %lu was correct for what is in Linus' tree.
Ah, you're absolutely correct. We have a local patch that makes
ion_phys_addr_t a dma_addr_t (needed for LPAE), which is why I needed to
convert the printk format...
Greg, can you please drop this patch from your tree? We'll only need
this if/when mainline Ion gets LPAE support...
-Mitch
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] ion: improve ion_phys error message
Clients often get confused when ion_phys errors out due to some heap
being used that they didn't expect. Add the heap name and heap type to
the error message to make it more obvious.
Signed-off-by: Mitchel Humpherys
---
drivers/staging/android/ion/ion.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 296d347660fc..966b7fdc9ecf 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -566,8 +566,8 @@ int ion_phys(struct ion_client *client, struct ion_handle
*handle,
buffer = handle->buffer;
if (!buffer->heap->ops->phys) {
- pr_err("%s: ion_phys is not implemented by this heap.\n",
- __func__);
+ pr_err("%s: ion_phys is not implemented by this heap (name=%s,
type=%d).\n",
+ __func__, buffer->heap->name, buffer->heap->type);
mutex_unlock(&client->lock);
return -ENODEV;
}
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] ion: improve ion_phys error message
Clients often get confused when ion_phys errors out due to some heap
being used that they didn't expect. Add the heap name and heap type to
the error message to make it more obvious.
Signed-off-by: Mitchel Humpherys
---
drivers/staging/android/ion/ion.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 296d347660fc..966b7fdc9ecf 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -566,8 +566,8 @@ int ion_phys(struct ion_client *client, struct ion_handle
*handle,
buffer = handle->buffer;
if (!buffer->heap->ops->phys) {
- pr_err("%s: ion_phys is not implemented by this heap.\n",
- __func__);
+ pr_err("%s: ion_phys is not implemented by this heap (name=%s,
type=%d).\n",
+ __func__, buffer->heap->name, buffer->heap->type);
mutex_unlock(&client->lock);
return -ENODEV;
}
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ion: always initialize the free list parameters
Currently we initialize the heap free_lock and free list size in
ion_heap_init_deferred_free, which is only called when the
ION_HEAP_FLAG_DEFER_FREE heap flag is given. However, the lock and size
are used in the shrinker path as well as the deferred free path, and we
can register a shrinker *without* enabling deferred freeing. So, if a
heap provides a shrinker but *doesn't* set the DEFER_FREE flag we will
use these parameters uninitialized (resulting in a spinlock bug and
broken shrinker accounting).
Fix these problems by initializing the free list parameters directly in
ion_device_add_heap, which is always called no matter which heap
features are being used.
Signed-off-by: Mitchel Humpherys
---
drivers/staging/android/ion/ion.c | 3 +++
drivers/staging/android/ion/ion_heap.c | 2 --
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 296d347660fc..b8f1c491553e 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -1508,6 +1508,9 @@ void ion_device_add_heap(struct ion_device *dev, struct
ion_heap *heap)
pr_err("%s: can not add heap with invalid ops struct.\n",
__func__);
+ spin_lock_init(&heap->free_lock);
+ heap->free_list_size = 0;
+
if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)
ion_heap_init_deferred_free(heap);
diff --git a/drivers/staging/android/ion/ion_heap.c
b/drivers/staging/android/ion/ion_heap.c
index 4605e04712aa..fd13d05b538a 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -253,8 +253,6 @@ int ion_heap_init_deferred_free(struct ion_heap *heap)
struct sched_param param = { .sched_priority = 0 };
INIT_LIST_HEAD(&heap->free_list);
- heap->free_list_size = 0;
- spin_lock_init(&heap->free_lock);
init_waitqueue_head(&heap->waitqueue);
heap->task = kthread_run(ion_heap_deferred_free, heap,
"%s", heap->name);
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
UAPI headers in staging drivers?
What's the protocol for UAPI headers in staging drivers? Specifically, I'm wondering how people usually install those headers. For example, there are a bunch of Android UAPI headers under drivers/staging/android/uapi which currently end up at /drivers with a call to `make headers_install'. All other UAPI headers end up at /usr so getting at the staging ones requires some special handling in the "outer" build system... To get the staging UAPI headers installed to /usr like the rest of the UAPI headers, we're currently running with something like this in our tree: --8<---cut here---start->8--- diff --git a/drivers/staging/android/uapi/Kbuild b/drivers/staging/android/uapi/Kbuild new file mode 100644 index 00..c075619bf4 --- /dev/null +++ b/drivers/staging/android/uapi/Kbuild @@ -0,0 +1 @@ +header-y += ion.h diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 6929571b79..8bba379192 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -20,6 +20,7 @@ header-y += netfilter_ipv4/ header-y += netfilter_ipv6/ header-y += usb/ header-y += wimax/ +header-y += ../../../drivers/staging/android/uapi/ genhdr-y += version.h diff --git a/scripts/Makefile.headersinst b/scripts/Makefile.headersinst index 8ccf83056a..79a64d7959 100644 --- a/scripts/Makefile.headersinst +++ b/scripts/Makefile.headersinst @@ -125,7 +125,7 @@ endif hdr-inst := -rR -f $(srctree)/scripts/Makefile.headersinst obj .PHONY: $(subdirs) $(subdirs): - $(Q)$(MAKE) $(hdr-inst)=$(obj)/$@ dst=$(_dst)/$@ + $(Q)$(MAKE) $(hdr-inst)=$(obj)/$@ dst=$(_dst)/$(subst drivers/staging,usr/include/linux/staging,$@) targets := $(wildcard $(sort $(targets))) cmd_files := $(wildcard \ --8<---cut here---end--->8--- With this, the headers end up in /usr/include/linux/staging instead of /drivers/staging. I know staging drivers are unstable but shouldn't they still be adhering to the UAPI split and shouldn't there be a way to install those split headers? How does everyone else deal with this issue? Thanks! Mitch -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/9] staging: ion: system heap and page pool fixes
On Tue, May 27 2014 at 11:52:51 PM, Heesub Shin wrote: > Hi, > > Here is my patchset with some modification, hoping reviews or comments > from you guys. > > v2: > o No changes in the code, just reworded changelog > o Reorder patch Some very nice cleanup. And I learned a new trick with page.lru :). Reviewed-by: Mitchel Humpherys > > Heesub Shin (9): > staging: ion: tidy up a bit > staging: ion: simplify ion_page_pool_total() > staging: ion: remove struct ion_page_pool_item > staging: ion: use compound pages on high order pages for system heap > staging: ion: remove order from struct page_info > staging: ion: remove struct page_info > staging: ion: remove order argument from free_buffer_page() > staging: ion: shrink highmem pages on kswapd > staging: ion: optimize struct ion_system_heap > > drivers/staging/android/ion/ion_page_pool.c | 49 --- > drivers/staging/android/ion/ion_priv.h| 2 +- > drivers/staging/android/ion/ion_system_heap.c | 121 > ++ > 3 files changed, 67 insertions(+), 105 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 7/9] staging: ion: remove order argument from free_buffer_page()
On Mon, May 26 2014 at 03:04:59 AM, Heesub Shin wrote: > Not that the pages returned from the pool are compound pages, we do not > need to pass the order information to free_buffer_page(). s/Not/Now/ -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/9] staging: ion: remove struct page_info
On Mon, May 26 2014 at 03:04:58 AM, Heesub Shin wrote:
> ION system heap uses a temporary list holding meta data on pages
> allocated to build scatter/gather table. Now that the pages are compound
> pages, we do not need to introduce a new data type redundantly.
>
> Signed-off-by: Heesub Shin
> ---
> drivers/staging/android/ion/ion_system_heap.c | 47
> +--
> 1 file changed, 15 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion_system_heap.c
> b/drivers/staging/android/ion/ion_system_heap.c
> index 73a2e67..f0ae210 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -51,11 +51,6 @@ struct ion_system_heap {
> struct ion_page_pool **pools;
> };
>
> -struct page_info {
> - struct page *page;
> - struct list_head list;
> -};
> -
> static struct page *alloc_buffer_page(struct ion_system_heap *heap,
> struct ion_buffer *buffer,
> unsigned long order)
> @@ -96,19 +91,14 @@ static void free_buffer_page(struct ion_system_heap *heap,
> }
>
>
> -static struct page_info *alloc_largest_available(struct ion_system_heap
> *heap,
> - struct ion_buffer *buffer,
> - unsigned long size,
> - unsigned int max_order)
> +static struct page *alloc_largest_available(struct ion_system_heap *heap,
> + struct ion_buffer *buffer,
> + unsigned long size,
> + unsigned int max_order)
Was this whitespace-only change intentional?
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RESEND PATCH] staging: ion: WARN when the handle kmap_cnt is going to wrap around
There are certain client bugs (double unmap, for example) that can cause
the handle->kmap_cnt (an unsigned int) to wrap around from zero. This
causes problems when the handle is destroyed because we have:
while (handle->kmap_cnt)
ion_handle_kmap_put(handle);
which takes a long time to complete when kmap_cnt starts at ~0 and can
result in a watchdog timeout.
WARN and bail when kmap_cnt is about to wrap around from zero.
Signed-off-by: Mitchel Humpherys
Acked-by: Colin Cross
---
Resending since I missed a few folks on the original. Also retaining
Colin's Acked-by.
---
drivers/staging/android/ion/ion.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 3d5bf14722..f55f61a4cc 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -626,6 +626,10 @@ static void ion_handle_kmap_put(struct ion_handle *handle)
{
struct ion_buffer *buffer = handle->buffer;
+ if (!handle->kmap_cnt) {
+ WARN(1, "%s: Double unmap detected! bailing...\n", __func__);
+ return;
+ }
handle->kmap_cnt--;
if (!handle->kmap_cnt)
ion_buffer_kmap_put(buffer);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: WARN when the handle kmap_cnt is going to wrap around
++greg-kh and de...@driverdev.osuosl.org
(my bad for missing you the first time around)
On Thu, May 22 2014 at 06:09:11 PM, Colin Cross wrote:
> On Thu, May 22, 2014 at 5:51 PM, Mitchel Humpherys
> wrote:
>> There are certain client bugs (double unmap, for example) that can cause
>> the handle->kmap_cnt (an unsigned int) to wrap around from zero. This
>> causes problems when the handle is destroyed because we have:
>>
>> while (handle->kmap_cnt)
>> ion_handle_kmap_put(handle);
>>
>> which takes a long time to complete when kmap_cnt starts at ~0 and can
>> result in a watchdog timeout.
>>
>> WARN and bail when kmap_cnt is about to wrap around from zero.
>>
>> Signed-off-by: Mitchel Humpherys
>> ---
>> drivers/staging/android/ion/ion.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index 3d5bf14722..f55f61a4cc 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -626,6 +626,10 @@ static void ion_handle_kmap_put(struct ion_handle
>> *handle)
>> {
>> struct ion_buffer *buffer = handle->buffer;
>>
>> + if (!handle->kmap_cnt) {
>> + WARN(1, "%s: Double unmap detected! bailing...\n", __func__);
>> + return;
>> + }
>> handle->kmap_cnt--;
>> if (!handle->kmap_cnt)
>> ion_buffer_kmap_put(buffer);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
>>
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to kernel-team+unsubscr...@android.com.
>
> Acked-by: Colin Cross
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
