Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1591485 > unrolled thread
| Started by | Laura Abbott <labbott@redhat.com> |
|---|---|
| First post | 2017-03-02 22:50 +0100 |
| Last post | 2017-03-03 20:20 +0100 |
| Articles | 20 on this page of 66 — 14 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
[RFC PATCH 01/12] staging: android: ion: Remove dmap_cnt Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
[RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
Re: [RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-03 17:50 +0100
Re: [RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Laura Abbott <labbott@redhat.com> - 2017-03-03 20:00 +0100
Re: [RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 11:50 +0100
Re: [RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-06 14:50 +0100
Re: [RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 17:00 +0100
Re: [RFC PATCH 10/12] staging: android: ion: Use CMA APIs directly Laura Abbott <labbott@redhat.com> - 2017-03-06 20:30 +0100
[RFC PATCH 09/12] cma: Introduce cma_for_each_area Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
[RFC PATCH 03/12] staging: android: ion: Duplicate sg_table Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
Re: [RFC PATCH 03/12] staging: android: ion: Duplicate sg_table "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-03-03 09:40 +0100
Re: [RFC PATCH 03/12] staging: android: ion: Duplicate sg_table Laura Abbott <labbott@redhat.com> - 2017-03-03 19:50 +0100
[RFC PATCH 11/12] staging: android: ion: Make Ion heaps selectable Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
Re: [RFC PATCH 11/12] staging: android: ion: Make Ion heaps selectable Daniel Vetter <daniel@ffwll.ch> - 2017-03-03 16:10 +0100
Re: [RFC PATCH 11/12] staging: android: ion: Make Ion heaps selectable Laura Abbott <labbott@redhat.com> - 2017-03-03 20:50 +0100
[RFC PATCH 05/12] staging: android: ion: Remove page faulting support Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
[RFC PATCH 02/12] staging: android: ion: Remove alignment from allocation field Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
[RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping Dan Carpenter <dan.carpenter@oracle.com> - 2017-03-03 12:10 +0100
Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping Eric Engestrom <eric.engestrom@imgtec.com> - 2017-03-03 13:00 +0100
Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-03 17:40 +0100
Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping Laura Abbott <labbott@redhat.com> - 2017-03-03 19:50 +0100
[RFC PATCH 07/12] staging: android: ion: Remove old platform support Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
Re: [RFC PATCH 07/12] staging: android: ion: Remove old platform support Daniel Vetter <daniel@ffwll.ch> - 2017-03-03 11:40 +0100
[RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Laura Abbott <labbott@redhat.com> - 2017-03-02 22:50 +0100
Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Daniel Vetter <daniel@ffwll.ch> - 2017-03-03 11:00 +0100
Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-03 18:00 +0100
Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Laura Abbott <labbott@redhat.com> - 2017-03-03 19:50 +0100
Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 11:50 +0100
Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Emil Velikov <emil.l.velikov@gmail.com> - 2017-03-06 18:40 +0100
Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support Laura Abbott <labbott@redhat.com> - 2017-03-06 20:30 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-03 11:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-03 11:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2017-03-03 15:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-03 17:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-03 20:20 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 11:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-06 16:10 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 17:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Michal Hocko <mhocko@kernel.org> - 2017-03-03 14:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-03 18:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Michal Hocko <mhocko@kernel.org> - 2017-03-06 09:10 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 12:00 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Mark Brown <broonie@kernel.org> - 2017-03-06 12:00 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-06 17:20 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2017-03-09 11:10 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-09 19:10 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Brian Starkey <brian.starkey@arm.com> - 2017-03-10 11:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Robin Murphy <robin.murphy@arm.com> - 2017-03-10 12:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Brian Starkey <brian.starkey@arm.com> - 2017-03-10 15:30 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-10 17:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel@ffwll.ch> - 2017-03-10 13:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Rob Clark <robdclark@gmail.com> - 2017-03-10 15:00 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Benjamin Gaignard <benjamin.gaignard@linaro.org> - 2017-03-12 14:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Daniel Vetter <daniel.vetter@ffwll.ch> - 2017-03-12 20:10 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-13 22:20 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Rob Clark <robdclark@gmail.com> - 2017-03-13 22:30 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-13 23:00 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Brian Starkey <brian.starkey@arm.com> - 2017-03-13 12:00 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Mark Brown <broonie@kernel.org> - 2017-03-13 14:30 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-13 22:50 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-13 22:30 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Michal Hocko <mhocko@kernel.org> - 2017-03-06 14:40 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-03 17:30 +0100
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging Laura Abbott <labbott@redhat.com> - 2017-03-03 20:20 +0100
Page 2 of 4 — ← Prev page 1 [2] 3 4 Next page →
| From | Eric Engestrom <eric.engestrom@imgtec.com> |
|---|---|
| Date | 2017-03-03 13:00 +0100 |
| Subject | Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping |
| Message-ID | <tgU2l-FV-21@gated-at.bofh.it> |
| In reply to | #1591861 |
On Friday, 2017-03-03 14:04:26 +0300, Dan Carpenter wrote:
> On Thu, Mar 02, 2017 at 01:44:36PM -0800, Laura Abbott wrote:
> > static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> > enum dma_data_direction direction)
> > {
> > struct dma_buf *dmabuf = attachment->dmabuf;
> > struct ion_buffer *buffer = dmabuf->priv;
> > + struct sg_table *table;
> > + int ret;
> > +
> > + /*
> > + * TODO: Need to sync wrt CPU or device completely owning?
> > + */
> > +
> > + table = dup_sg_table(buffer->sg_table);
> >
> > - ion_buffer_sync_for_device(buffer, attachment->dev, direction);
> > - return dup_sg_table(buffer->sg_table);
> > + if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> > + direction)){
> > + ret = -ENOMEM;
> > + goto err;
> > + }
Actually, I think `ret` should be left uninitialised on success,
what's really missing is this return before the `err:` label:
+ return table;
> > +
> > +err:
> > + free_duped_table(table);
> > + return ERR_PTR(ret);
>
> ret isn't initialized on success.
>
> > }
> >
>
> regards,
> dan carpenter
[toc] | [prev] | [next] | [standalone]
| From | Laurent Pinchart <laurent.pinchart@ideasonboard.com> |
|---|---|
| Date | 2017-03-03 17:40 +0100 |
| Subject | Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping |
| Message-ID | <tgYpk-3NK-11@gated-at.bofh.it> |
| In reply to | #1591495 |
Hi Laura,
Thank you for the patch.
On Thursday 02 Mar 2017 13:44:36 Laura Abbott wrote:
> Technically, calling dma_buf_map_attachment should return a buffer
> properly dma_mapped. Add calls to dma_map_sg to begin_cpu_access to
> ensure this happens. As a side effect, this lets Ion buffers take
> advantage of the dma_buf sync ioctls.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> drivers/staging/android/ion/ion.c | 101 +++++++++++++++++------------------
> 1 file changed, 50 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c index ce4adac..a931b30 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -795,10 +795,6 @@ void ion_client_destroy(struct ion_client *client)
> }
> EXPORT_SYMBOL(ion_client_destroy);
>
> -static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
> - struct device *dev,
> - enum dma_data_direction direction);
> -
> static struct sg_table *dup_sg_table(struct sg_table *table)
> {
> struct sg_table *new_table;
> @@ -825,22 +821,43 @@ static struct sg_table *dup_sg_table(struct sg_table
> *table) return new_table;
> }
>
> +static void free_duped_table(struct sg_table *table)
> +{
> + sg_free_table(table);
> + kfree(table);
> +}
> +
> static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment
> *attachment, enum dma_data_direction direction)
> {
> struct dma_buf *dmabuf = attachment->dmabuf;
> struct ion_buffer *buffer = dmabuf->priv;
> + struct sg_table *table;
> + int ret;
> +
> + /*
> + * TODO: Need to sync wrt CPU or device completely owning?
> + */
> +
> + table = dup_sg_table(buffer->sg_table);
>
> - ion_buffer_sync_for_device(buffer, attachment->dev, direction);
> - return dup_sg_table(buffer->sg_table);
> + if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> + direction)){
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> +err:
> + free_duped_table(table);
> + return ERR_PTR(ret);
> }
>
> static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
> struct sg_table *table,
> enum dma_data_direction direction)
> {
> - sg_free_table(table);
> - kfree(table);
> + dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> + free_duped_table(table);
> }
>
> void ion_pages_sync_for_device(struct device *dev, struct page *page,
> @@ -864,38 +881,6 @@ struct ion_vma_list {
> struct vm_area_struct *vma;
> };
>
> -static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
> - struct device *dev,
> - enum dma_data_direction dir)
> -{
> - struct ion_vma_list *vma_list;
> - int pages = PAGE_ALIGN(buffer->size) / PAGE_SIZE;
> - int i;
> -
> - pr_debug("%s: syncing for device %s\n", __func__,
> - dev ? dev_name(dev) : "null");
> -
> - if (!ion_buffer_fault_user_mappings(buffer))
> - return;
> -
> - mutex_lock(&buffer->lock);
> - for (i = 0; i < pages; i++) {
> - struct page *page = buffer->pages[i];
> -
> - if (ion_buffer_page_is_dirty(page))
> - ion_pages_sync_for_device(dev, ion_buffer_page(page),
> - PAGE_SIZE, dir);
> -
> - ion_buffer_page_clean(buffer->pages + i);
> - }
> - list_for_each_entry(vma_list, &buffer->vmas, list) {
> - struct vm_area_struct *vma = vma_list->vma;
> -
> - zap_page_range(vma, vma->vm_start, vma->vm_end - vma-
>vm_start);
> - }
> - mutex_unlock(&buffer->lock);
> -}
> -
> static int ion_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> {
> struct ion_buffer *buffer = vma->vm_private_data;
> @@ -1014,16 +999,24 @@ static int ion_dma_buf_begin_cpu_access(struct
> dma_buf *dmabuf, struct ion_buffer *buffer = dmabuf->priv;
> void *vaddr;
>
> - if (!buffer->heap->ops->map_kernel) {
> - pr_err("%s: map kernel is not implemented by this heap.\n",
> - __func__);
> - return -ENODEV;
> + /*
> + * TODO: Move this elsewhere because we don't always need a vaddr
> + */
> + if (buffer->heap->ops->map_kernel) {
> + mutex_lock(&buffer->lock);
> + vaddr = ion_buffer_kmap_get(buffer);
> + mutex_unlock(&buffer->lock);
> }
>
> - mutex_lock(&buffer->lock);
> - vaddr = ion_buffer_kmap_get(buffer);
> - mutex_unlock(&buffer->lock);
> - return PTR_ERR_OR_ZERO(vaddr);
> + /*
> + * Close enough right now? Flag to skip sync?
> + */
> + if (!dma_map_sg(buffer->dev->dev.this_device, buffer->sg_table->sgl,
> + buffer->sg_table->nents,
> + DMA_BIDIRECTIONAL))
Aren't the dma_(un)map_* calls supposed to take a real, physical device as
their first argument ? Beside, this doesn't seem to be the right place to
create the mapping, as you mentioned in the commit message the buffer should
be mapped in the dma_buf map handler. This is something that needs to be
fixed, especially in the light of the comment in ion_buffer_create():
/*
* this will set up dma addresses for the sglist -- it is not
* technically correct as per the dma api -- a specific
* device isn't really taking ownership here. However, in practice on
* our systems the only dma_address space is physical addresses.
* Additionally, we can't afford the overhead of invalidating every
* allocation via dma_map_sg. The implicit contract here is that
* memory coming from the heaps is ready for dma, ie if it has a
* cached mapping that mapping has been invalidated
*/
That's a showstopper in my opinion, the DMA address space can't be restricted
to physical addresses, IOMMU have to be supported.
> + return -ENOMEM;
> +
> + return 0;
> }
>
> static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> @@ -1031,9 +1024,15 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf
> *dmabuf, {
> struct ion_buffer *buffer = dmabuf->priv;
>
> - mutex_lock(&buffer->lock);
> - ion_buffer_kmap_put(buffer);
> - mutex_unlock(&buffer->lock);
> + if (buffer->heap->ops->map_kernel) {
> + mutex_lock(&buffer->lock);
> + ion_buffer_kmap_put(buffer);
> + mutex_unlock(&buffer->lock);
> + }
> +
> + dma_unmap_sg(buffer->dev->dev.this_device, buffer->sg_table->sgl,
> + buffer->sg_table->nents,
> + DMA_BIDIRECTIONAL);
>
> return 0;
> }
--
Regards,
Laurent Pinchart
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2017-03-03 19:50 +0100 |
| Subject | Re: [RFC PATCH 04/12] staging: android: ion: Call dma_map_sg for syncing and mapping |
| Message-ID | <th0r9-5c2-37@gated-at.bofh.it> |
| In reply to | #1592124 |
On 03/03/2017 08:37 AM, Laurent Pinchart wrote:
> Hi Laura,
>
> Thank you for the patch.
>
> On Thursday 02 Mar 2017 13:44:36 Laura Abbott wrote:
>> Technically, calling dma_buf_map_attachment should return a buffer
>> properly dma_mapped. Add calls to dma_map_sg to begin_cpu_access to
>> ensure this happens. As a side effect, this lets Ion buffers take
>> advantage of the dma_buf sync ioctls.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> drivers/staging/android/ion/ion.c | 101 +++++++++++++++++------------------
>> 1 file changed, 50 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c index ce4adac..a931b30 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -795,10 +795,6 @@ void ion_client_destroy(struct ion_client *client)
>> }
>> EXPORT_SYMBOL(ion_client_destroy);
>>
>> -static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
>> - struct device *dev,
>> - enum dma_data_direction direction);
>> -
>> static struct sg_table *dup_sg_table(struct sg_table *table)
>> {
>> struct sg_table *new_table;
>> @@ -825,22 +821,43 @@ static struct sg_table *dup_sg_table(struct sg_table
>> *table) return new_table;
>> }
>>
>> +static void free_duped_table(struct sg_table *table)
>> +{
>> + sg_free_table(table);
>> + kfree(table);
>> +}
>> +
>> static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment
>> *attachment, enum dma_data_direction direction)
>> {
>> struct dma_buf *dmabuf = attachment->dmabuf;
>> struct ion_buffer *buffer = dmabuf->priv;
>> + struct sg_table *table;
>> + int ret;
>> +
>> + /*
>> + * TODO: Need to sync wrt CPU or device completely owning?
>> + */
>> +
>> + table = dup_sg_table(buffer->sg_table);
>>
>> - ion_buffer_sync_for_device(buffer, attachment->dev, direction);
>> - return dup_sg_table(buffer->sg_table);
>> + if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
>> + direction)){
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> +err:
>> + free_duped_table(table);
>> + return ERR_PTR(ret);
>> }
>>
>> static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
>> struct sg_table *table,
>> enum dma_data_direction direction)
>> {
>> - sg_free_table(table);
>> - kfree(table);
>> + dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
>> + free_duped_table(table);
>> }
>>
>> void ion_pages_sync_for_device(struct device *dev, struct page *page,
>> @@ -864,38 +881,6 @@ struct ion_vma_list {
>> struct vm_area_struct *vma;
>> };
>>
>> -static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
>> - struct device *dev,
>> - enum dma_data_direction dir)
>> -{
>> - struct ion_vma_list *vma_list;
>> - int pages = PAGE_ALIGN(buffer->size) / PAGE_SIZE;
>> - int i;
>> -
>> - pr_debug("%s: syncing for device %s\n", __func__,
>> - dev ? dev_name(dev) : "null");
>> -
>> - if (!ion_buffer_fault_user_mappings(buffer))
>> - return;
>> -
>> - mutex_lock(&buffer->lock);
>> - for (i = 0; i < pages; i++) {
>> - struct page *page = buffer->pages[i];
>> -
>> - if (ion_buffer_page_is_dirty(page))
>> - ion_pages_sync_for_device(dev, ion_buffer_page(page),
>> - PAGE_SIZE, dir);
>> -
>> - ion_buffer_page_clean(buffer->pages + i);
>> - }
>> - list_for_each_entry(vma_list, &buffer->vmas, list) {
>> - struct vm_area_struct *vma = vma_list->vma;
>> -
>> - zap_page_range(vma, vma->vm_start, vma->vm_end - vma-
>> vm_start);
>> - }
>> - mutex_unlock(&buffer->lock);
>> -}
>> -
>> static int ion_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> {
>> struct ion_buffer *buffer = vma->vm_private_data;
>> @@ -1014,16 +999,24 @@ static int ion_dma_buf_begin_cpu_access(struct
>> dma_buf *dmabuf, struct ion_buffer *buffer = dmabuf->priv;
>> void *vaddr;
>>
>> - if (!buffer->heap->ops->map_kernel) {
>> - pr_err("%s: map kernel is not implemented by this heap.\n",
>> - __func__);
>> - return -ENODEV;
>> + /*
>> + * TODO: Move this elsewhere because we don't always need a vaddr
>> + */
>> + if (buffer->heap->ops->map_kernel) {
>> + mutex_lock(&buffer->lock);
>> + vaddr = ion_buffer_kmap_get(buffer);
>> + mutex_unlock(&buffer->lock);
>> }
>>
>> - mutex_lock(&buffer->lock);
>> - vaddr = ion_buffer_kmap_get(buffer);
>> - mutex_unlock(&buffer->lock);
>> - return PTR_ERR_OR_ZERO(vaddr);
>> + /*
>> + * Close enough right now? Flag to skip sync?
>> + */
>> + if (!dma_map_sg(buffer->dev->dev.this_device, buffer->sg_table->sgl,
>> + buffer->sg_table->nents,
>> + DMA_BIDIRECTIONAL))
>
> Aren't the dma_(un)map_* calls supposed to take a real, physical device as
> their first argument ? Beside, this doesn't seem to be the right place to
> create the mapping, as you mentioned in the commit message the buffer should
> be mapped in the dma_buf map handler. This is something that needs to be
> fixed, especially in the light of the comment in ion_buffer_create():
>
Yes, this might me a case of me getting the model incorrect again.
dma_buf_{begin,end}_cpu_access do not take a device structure and
from the comments:
/**
* dma_buf_begin_cpu_access - Must be called before accessing a dma_buf from the
* cpu in the kernel context. Calls begin_cpu_access to allow exporter-specific
* preparations. Coherency is only guaranteed in the specified range for the
* specified access direction.
* @dmabuf: [in] buffer to prepare cpu access for.
* @direction: [in] length of range for cpu access.
*
* Can return negative error values, returns 0 on success.
*/
If there are no buffer attachments, I guess the notion of 'coherency'
doesn't apply here so there is no need to do any kind of
syncing/mapping at all vs. trying to find a device out of nowhere.
I'll have to go back and re-think aligning sync/begin_cpu_access calls
and dma_buf_map calls, or more likely not overthink this.
> /*
> * this will set up dma addresses for the sglist -- it is not
> * technically correct as per the dma api -- a specific
> * device isn't really taking ownership here. However, in practice on
> * our systems the only dma_address space is physical addresses.
> * Additionally, we can't afford the overhead of invalidating every
> * allocation via dma_map_sg. The implicit contract here is that
> * memory coming from the heaps is ready for dma, ie if it has a
> * cached mapping that mapping has been invalidated
> */
>
> That's a showstopper in my opinion, the DMA address space can't be restricted
> to physical addresses, IOMMU have to be supported.
>
I missed a patch in this series to remove that. If Ion is going to exist outside
of staging it should not be making that assumption at all so I want to drop it.
Any performance implications should be fixed with the skip sync flag.
Thanks,
Laura
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2017-03-02 22:50 +0100 |
| Subject | [RFC PATCH 07/12] staging: android: ion: Remove old platform support |
| Message-ID | <tgGLM-8cb-27@gated-at.bofh.it> |
| In reply to | #1591485 |
Device specific platform support has been haphazard for Ion. There have
been several independent attempts and there are still objections to
what bindings exist right now. Just remove everything for a fresh start.
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
drivers/staging/android/ion/Kconfig | 35 ----
drivers/staging/android/ion/Makefile | 6 -
drivers/staging/android/ion/hisilicon/Kconfig | 5 -
drivers/staging/android/ion/hisilicon/Makefile | 1 -
drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 -------------
drivers/staging/android/ion/ion_dummy_driver.c | 156 -----------------
drivers/staging/android/ion/ion_of.c | 184 ---------------------
drivers/staging/android/ion/ion_of.h | 37 -----
drivers/staging/android/ion/tegra/Makefile | 1 -
drivers/staging/android/ion/tegra/tegra_ion.c | 80 ---------
10 files changed, 618 deletions(-)
delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig
delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile
delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c
delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c
delete mode 100644 drivers/staging/android/ion/ion_of.c
delete mode 100644 drivers/staging/android/ion/ion_of.h
delete mode 100644 drivers/staging/android/ion/tegra/Makefile
delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c
diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index c8fb413..0c91b2b 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -17,38 +17,3 @@ config ION_TEST
Choose this option to create a device that can be used to test the
kernel and device side ION functions.
-config ION_DUMMY
- bool "Dummy Ion driver"
- depends on ION
- help
- Provides a dummy ION driver that registers the
- /dev/ion device and some basic heaps. This can
- be used for testing the ION infrastructure if
- one doesn't have access to hardware drivers that
- use ION.
-
-config ION_TEGRA
- tristate "Ion for Tegra"
- depends on ARCH_TEGRA && ION
- help
- Choose this option if you wish to use ion on an nVidia Tegra.
-
-config ION_HISI
- tristate "Ion for Hisilicon"
- depends on ARCH_HISI && ION
- select ION_OF
- help
- Choose this option if you wish to use ion on Hisilicon Platform.
-
-source "drivers/staging/android/ion/hisilicon/Kconfig"
-
-config ION_OF
- bool "Devicetree support for Ion"
- depends on ION && OF_ADDRESS
- help
- Provides base support for defining Ion heaps in devicetree
- and setting them up. Also includes functions for platforms
- to parse the devicetree and expand for their own custom
- extensions
-
- If using Ion and devicetree, you should say Y here
diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
index 5d630a0..9457090 100644
--- a/drivers/staging/android/ion/Makefile
+++ b/drivers/staging/android/ion/Makefile
@@ -5,9 +5,3 @@ obj-$(CONFIG_ION_TEST) += ion_test.o
ifdef CONFIG_COMPAT
obj-$(CONFIG_ION) += compat_ion.o
endif
-
-obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
-obj-$(CONFIG_ION_TEGRA) += tegra/
-obj-$(CONFIG_ION_HISI) += hisilicon/
-obj-$(CONFIG_ION_OF) += ion_of.o
-
diff --git a/drivers/staging/android/ion/hisilicon/Kconfig b/drivers/staging/android/ion/hisilicon/Kconfig
deleted file mode 100644
index 2b4bd07..0000000
--- a/drivers/staging/android/ion/hisilicon/Kconfig
+++ /dev/null
@@ -1,5 +0,0 @@
-config HI6220_ION
- bool "Hi6220 ION Driver"
- depends on ARCH_HISI && ION
- help
- Build the Hisilicon Hi6220 ion driver.
diff --git a/drivers/staging/android/ion/hisilicon/Makefile b/drivers/staging/android/ion/hisilicon/Makefile
deleted file mode 100644
index 2a89414..0000000
--- a/drivers/staging/android/ion/hisilicon/Makefile
+++ /dev/null
@@ -1 +0,0 @@
-obj-$(CONFIG_HI6220_ION) += hi6220_ion.o
diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
deleted file mode 100644
index 0de7897..0000000
--- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c
+++ /dev/null
@@ -1,113 +0,0 @@
-/*
- * Hisilicon Hi6220 ION Driver
- *
- * Copyright (c) 2015 Hisilicon Limited.
- *
- * Author: Chen Feng <puck.chen@hisilicon.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#define pr_fmt(fmt) "Ion: " fmt
-
-#include <linux/err.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/of.h>
-#include <linux/mm.h>
-#include "../ion_priv.h"
-#include "../ion.h"
-#include "../ion_of.h"
-
-struct hisi_ion_dev {
- struct ion_heap **heaps;
- struct ion_device *idev;
- struct ion_platform_data *data;
-};
-
-static struct ion_of_heap hisi_heaps[] = {
- PLATFORM_HEAP("hisilicon,sys_user", 0,
- ION_HEAP_TYPE_SYSTEM, "sys_user"),
- PLATFORM_HEAP("hisilicon,sys_contig", 1,
- ION_HEAP_TYPE_SYSTEM_CONTIG, "sys_contig"),
- PLATFORM_HEAP("hisilicon,cma", ION_HEAP_TYPE_DMA, ION_HEAP_TYPE_DMA,
- "cma"),
- {}
-};
-
-static int hi6220_ion_probe(struct platform_device *pdev)
-{
- struct hisi_ion_dev *ipdev;
- int i;
-
- ipdev = devm_kzalloc(&pdev->dev, sizeof(*ipdev), GFP_KERNEL);
- if (!ipdev)
- return -ENOMEM;
-
- platform_set_drvdata(pdev, ipdev);
-
- ipdev->idev = ion_device_create(NULL);
- if (IS_ERR(ipdev->idev))
- return PTR_ERR(ipdev->idev);
-
- ipdev->data = ion_parse_dt(pdev, hisi_heaps);
- if (IS_ERR(ipdev->data))
- return PTR_ERR(ipdev->data);
-
- ipdev->heaps = devm_kzalloc(&pdev->dev,
- sizeof(struct ion_heap) * ipdev->data->nr,
- GFP_KERNEL);
- if (!ipdev->heaps) {
- ion_destroy_platform_data(ipdev->data);
- return -ENOMEM;
- }
-
- for (i = 0; i < ipdev->data->nr; i++) {
- ipdev->heaps[i] = ion_heap_create(&ipdev->data->heaps[i]);
- if (!ipdev->heaps) {
- ion_destroy_platform_data(ipdev->data);
- return -ENOMEM;
- }
- ion_device_add_heap(ipdev->idev, ipdev->heaps[i]);
- }
- return 0;
-}
-
-static int hi6220_ion_remove(struct platform_device *pdev)
-{
- struct hisi_ion_dev *ipdev;
- int i;
-
- ipdev = platform_get_drvdata(pdev);
-
- for (i = 0; i < ipdev->data->nr; i++)
- ion_heap_destroy(ipdev->heaps[i]);
-
- ion_destroy_platform_data(ipdev->data);
- ion_device_destroy(ipdev->idev);
-
- return 0;
-}
-
-static const struct of_device_id hi6220_ion_match_table[] = {
- {.compatible = "hisilicon,hi6220-ion"},
- {},
-};
-
-static struct platform_driver hi6220_ion_driver = {
- .probe = hi6220_ion_probe,
- .remove = hi6220_ion_remove,
- .driver = {
- .name = "ion-hi6220",
- .of_match_table = hi6220_ion_match_table,
- },
-};
-
-static int __init hi6220_ion_init(void)
-{
- return platform_driver_register(&hi6220_ion_driver);
-}
-
-subsys_initcall(hi6220_ion_init);
diff --git a/drivers/staging/android/ion/ion_dummy_driver.c b/drivers/staging/android/ion/ion_dummy_driver.c
deleted file mode 100644
index cf5c010..0000000
--- a/drivers/staging/android/ion/ion_dummy_driver.c
+++ /dev/null
@@ -1,156 +0,0 @@
-/*
- * drivers/gpu/ion/ion_dummy_driver.c
- *
- * Copyright (C) 2013 Linaro, Inc
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/err.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/init.h>
-#include <linux/bootmem.h>
-#include <linux/memblock.h>
-#include <linux/sizes.h>
-#include <linux/io.h>
-#include "ion.h"
-#include "ion_priv.h"
-
-static struct ion_device *idev;
-static struct ion_heap **heaps;
-
-static void *carveout_ptr;
-static void *chunk_ptr;
-
-static struct ion_platform_heap dummy_heaps[] = {
- {
- .id = ION_HEAP_TYPE_SYSTEM,
- .type = ION_HEAP_TYPE_SYSTEM,
- .name = "system",
- },
- {
- .id = ION_HEAP_TYPE_SYSTEM_CONTIG,
- .type = ION_HEAP_TYPE_SYSTEM_CONTIG,
- .name = "system contig",
- },
- {
- .id = ION_HEAP_TYPE_CARVEOUT,
- .type = ION_HEAP_TYPE_CARVEOUT,
- .name = "carveout",
- .size = SZ_4M,
- },
- {
- .id = ION_HEAP_TYPE_CHUNK,
- .type = ION_HEAP_TYPE_CHUNK,
- .name = "chunk",
- .size = SZ_4M,
- .align = SZ_16K,
- .priv = (void *)(SZ_16K),
- },
-};
-
-static const struct ion_platform_data dummy_ion_pdata = {
- .nr = ARRAY_SIZE(dummy_heaps),
- .heaps = dummy_heaps,
-};
-
-static int __init ion_dummy_init(void)
-{
- int i, err;
-
- idev = ion_device_create(NULL);
- if (IS_ERR(idev))
- return PTR_ERR(idev);
- heaps = kcalloc(dummy_ion_pdata.nr, sizeof(struct ion_heap *),
- GFP_KERNEL);
- if (!heaps)
- return -ENOMEM;
-
-
- /* Allocate a dummy carveout heap */
- carveout_ptr = alloc_pages_exact(
- dummy_heaps[ION_HEAP_TYPE_CARVEOUT].size,
- GFP_KERNEL);
- if (carveout_ptr)
- dummy_heaps[ION_HEAP_TYPE_CARVEOUT].base =
- virt_to_phys(carveout_ptr);
- else
- pr_err("ion_dummy: Could not allocate carveout\n");
-
- /* Allocate a dummy chunk heap */
- chunk_ptr = alloc_pages_exact(
- dummy_heaps[ION_HEAP_TYPE_CHUNK].size,
- GFP_KERNEL);
- if (chunk_ptr)
- dummy_heaps[ION_HEAP_TYPE_CHUNK].base = virt_to_phys(chunk_ptr);
- else
- pr_err("ion_dummy: Could not allocate chunk\n");
-
- for (i = 0; i < dummy_ion_pdata.nr; i++) {
- struct ion_platform_heap *heap_data = &dummy_ion_pdata.heaps[i];
-
- if (heap_data->type == ION_HEAP_TYPE_CARVEOUT &&
- !heap_data->base)
- continue;
-
- if (heap_data->type == ION_HEAP_TYPE_CHUNK && !heap_data->base)
- continue;
-
- heaps[i] = ion_heap_create(heap_data);
- if (IS_ERR_OR_NULL(heaps[i])) {
- err = PTR_ERR(heaps[i]);
- goto err;
- }
- ion_device_add_heap(idev, heaps[i]);
- }
- return 0;
-err:
- for (i = 0; i < dummy_ion_pdata.nr; ++i)
- ion_heap_destroy(heaps[i]);
- kfree(heaps);
-
- if (carveout_ptr) {
- free_pages_exact(carveout_ptr,
- dummy_heaps[ION_HEAP_TYPE_CARVEOUT].size);
- carveout_ptr = NULL;
- }
- if (chunk_ptr) {
- free_pages_exact(chunk_ptr,
- dummy_heaps[ION_HEAP_TYPE_CHUNK].size);
- chunk_ptr = NULL;
- }
- return err;
-}
-device_initcall(ion_dummy_init);
-
-static void __exit ion_dummy_exit(void)
-{
- int i;
-
- ion_device_destroy(idev);
-
- for (i = 0; i < dummy_ion_pdata.nr; i++)
- ion_heap_destroy(heaps[i]);
- kfree(heaps);
-
- if (carveout_ptr) {
- free_pages_exact(carveout_ptr,
- dummy_heaps[ION_HEAP_TYPE_CARVEOUT].size);
- carveout_ptr = NULL;
- }
- if (chunk_ptr) {
- free_pages_exact(chunk_ptr,
- dummy_heaps[ION_HEAP_TYPE_CHUNK].size);
- chunk_ptr = NULL;
- }
-}
-__exitcall(ion_dummy_exit);
diff --git a/drivers/staging/android/ion/ion_of.c b/drivers/staging/android/ion/ion_of.c
deleted file mode 100644
index 7791c70..0000000
--- a/drivers/staging/android/ion/ion_of.c
+++ /dev/null
@@ -1,184 +0,0 @@
-/*
- * Based on work from:
- * Andrew Andrianov <andrew@ncrmnt.org>
- * Google
- * The Linux Foundation
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include <linux/init.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/of.h>
-#include <linux/of_platform.h>
-#include <linux/of_address.h>
-#include <linux/clk.h>
-#include <linux/dma-mapping.h>
-#include <linux/cma.h>
-#include <linux/dma-contiguous.h>
-#include <linux/io.h>
-#include <linux/of_reserved_mem.h>
-#include "ion.h"
-#include "ion_priv.h"
-#include "ion_of.h"
-
-static int ion_parse_dt_heap_common(struct device_node *heap_node,
- struct ion_platform_heap *heap,
- struct ion_of_heap *compatible)
-{
- int i;
-
- for (i = 0; compatible[i].name; i++) {
- if (of_device_is_compatible(heap_node, compatible[i].compat))
- break;
- }
-
- if (!compatible[i].name)
- return -ENODEV;
-
- heap->id = compatible[i].heap_id;
- heap->type = compatible[i].type;
- heap->name = compatible[i].name;
- heap->align = compatible[i].align;
-
- /* Some kind of callback function pointer? */
-
- pr_info("%s: id %d type %d name %s align %lx\n", __func__,
- heap->id, heap->type, heap->name, heap->align);
- return 0;
-}
-
-static int ion_setup_heap_common(struct platform_device *parent,
- struct device_node *heap_node,
- struct ion_platform_heap *heap)
-{
- int ret = 0;
-
- switch (heap->type) {
- case ION_HEAP_TYPE_CARVEOUT:
- case ION_HEAP_TYPE_CHUNK:
- if (heap->base && heap->size)
- return 0;
-
- ret = of_reserved_mem_device_init(heap->priv);
- break;
- default:
- break;
- }
-
- return ret;
-}
-
-struct ion_platform_data *ion_parse_dt(struct platform_device *pdev,
- struct ion_of_heap *compatible)
-{
- int num_heaps, ret;
- const struct device_node *dt_node = pdev->dev.of_node;
- struct device_node *node;
- struct ion_platform_heap *heaps;
- struct ion_platform_data *data;
- int i = 0;
-
- num_heaps = of_get_available_child_count(dt_node);
-
- if (!num_heaps)
- return ERR_PTR(-EINVAL);
-
- heaps = devm_kzalloc(&pdev->dev,
- sizeof(struct ion_platform_heap) * num_heaps,
- GFP_KERNEL);
- if (!heaps)
- return ERR_PTR(-ENOMEM);
-
- data = devm_kzalloc(&pdev->dev, sizeof(struct ion_platform_data),
- GFP_KERNEL);
- if (!data)
- return ERR_PTR(-ENOMEM);
-
- for_each_available_child_of_node(dt_node, node) {
- struct platform_device *heap_pdev;
-
- ret = ion_parse_dt_heap_common(node, &heaps[i], compatible);
- if (ret)
- return ERR_PTR(ret);
-
- heap_pdev = of_platform_device_create(node, heaps[i].name,
- &pdev->dev);
- if (!heap_pdev)
- return ERR_PTR(-ENOMEM);
- heap_pdev->dev.platform_data = &heaps[i];
-
- heaps[i].priv = &heap_pdev->dev;
-
- ret = ion_setup_heap_common(pdev, node, &heaps[i]);
- if (ret)
- goto out_err;
- i++;
- }
-
- data->heaps = heaps;
- data->nr = num_heaps;
- return data;
-
-out_err:
- for ( ; i >= 0; i--)
- if (heaps[i].priv)
- of_device_unregister(to_platform_device(heaps[i].priv));
-
- return ERR_PTR(ret);
-}
-
-void ion_destroy_platform_data(struct ion_platform_data *data)
-{
- int i;
-
- for (i = 0; i < data->nr; i++)
- if (data->heaps[i].priv)
- of_device_unregister(to_platform_device(
- data->heaps[i].priv));
-}
-
-#ifdef CONFIG_OF_RESERVED_MEM
-#include <linux/of.h>
-#include <linux/of_fdt.h>
-#include <linux/of_reserved_mem.h>
-
-static int rmem_ion_device_init(struct reserved_mem *rmem, struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ion_platform_heap *heap = pdev->dev.platform_data;
-
- heap->base = rmem->base;
- heap->base = rmem->size;
- pr_debug("%s: heap %s base %pa size %pa dev %p\n", __func__,
- heap->name, &rmem->base, &rmem->size, dev);
- return 0;
-}
-
-static void rmem_ion_device_release(struct reserved_mem *rmem,
- struct device *dev)
-{
-}
-
-static const struct reserved_mem_ops rmem_dma_ops = {
- .device_init = rmem_ion_device_init,
- .device_release = rmem_ion_device_release,
-};
-
-static int __init rmem_ion_setup(struct reserved_mem *rmem)
-{
- phys_addr_t size = rmem->size;
-
- size = size / 1024;
-
- pr_info("Ion memory setup at %pa size %pa MiB\n",
- &rmem->base, &size);
- rmem->ops = &rmem_dma_ops;
- return 0;
-}
-
-RESERVEDMEM_OF_DECLARE(ion, "ion-region", rmem_ion_setup);
-#endif
diff --git a/drivers/staging/android/ion/ion_of.h b/drivers/staging/android/ion/ion_of.h
deleted file mode 100644
index 8241a17..0000000
--- a/drivers/staging/android/ion/ion_of.h
+++ /dev/null
@@ -1,37 +0,0 @@
-/*
- * Based on work from:
- * Andrew Andrianov <andrew@ncrmnt.org>
- * Google
- * The Linux Foundation
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#ifndef _ION_OF_H
-#define _ION_OF_H
-
-struct ion_of_heap {
- const char *compat;
- int heap_id;
- int type;
- const char *name;
- int align;
-};
-
-#define PLATFORM_HEAP(_compat, _id, _type, _name) \
-{ \
- .compat = _compat, \
- .heap_id = _id, \
- .type = _type, \
- .name = _name, \
- .align = PAGE_SIZE, \
-}
-
-struct ion_platform_data *ion_parse_dt(struct platform_device *pdev,
- struct ion_of_heap *compatible);
-
-void ion_destroy_platform_data(struct ion_platform_data *data);
-
-#endif
diff --git a/drivers/staging/android/ion/tegra/Makefile b/drivers/staging/android/ion/tegra/Makefile
deleted file mode 100644
index 808f1f5..0000000
--- a/drivers/staging/android/ion/tegra/Makefile
+++ /dev/null
@@ -1 +0,0 @@
-obj-$(CONFIG_ION_TEGRA) += tegra_ion.o
diff --git a/drivers/staging/android/ion/tegra/tegra_ion.c b/drivers/staging/android/ion/tegra/tegra_ion.c
deleted file mode 100644
index 49e55e5..0000000
--- a/drivers/staging/android/ion/tegra/tegra_ion.c
+++ /dev/null
@@ -1,80 +0,0 @@
-/*
- * drivers/gpu/tegra/tegra_ion.c
- *
- * Copyright (C) 2011 Google, Inc.
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/err.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include "../ion.h"
-#include "../ion_priv.h"
-
-static struct ion_device *idev;
-static int num_heaps;
-static struct ion_heap **heaps;
-
-static int tegra_ion_probe(struct platform_device *pdev)
-{
- struct ion_platform_data *pdata = pdev->dev.platform_data;
- int err;
- int i;
-
- num_heaps = pdata->nr;
-
- heaps = devm_kcalloc(&pdev->dev, pdata->nr,
- sizeof(struct ion_heap *), GFP_KERNEL);
-
- idev = ion_device_create(NULL);
- if (IS_ERR(idev))
- return PTR_ERR(idev);
-
- /* create the heaps as specified in the board file */
- for (i = 0; i < num_heaps; i++) {
- struct ion_platform_heap *heap_data = &pdata->heaps[i];
-
- heaps[i] = ion_heap_create(heap_data);
- if (IS_ERR_OR_NULL(heaps[i])) {
- err = PTR_ERR(heaps[i]);
- goto err;
- }
- ion_device_add_heap(idev, heaps[i]);
- }
- platform_set_drvdata(pdev, idev);
- return 0;
-err:
- for (i = 0; i < num_heaps; ++i)
- ion_heap_destroy(heaps[i]);
- return err;
-}
-
-static int tegra_ion_remove(struct platform_device *pdev)
-{
- struct ion_device *idev = platform_get_drvdata(pdev);
- int i;
-
- ion_device_destroy(idev);
- for (i = 0; i < num_heaps; i++)
- ion_heap_destroy(heaps[i]);
- return 0;
-}
-
-static struct platform_driver ion_driver = {
- .probe = tegra_ion_probe,
- .remove = tegra_ion_remove,
- .driver = { .name = "ion-tegra" }
-};
-
-module_platform_driver(ion_driver);
-
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-03 11:40 +0100 |
| Subject | Re: [RFC PATCH 07/12] staging: android: ion: Remove old platform support |
| Message-ID | <tgSMV-8mQ-5@gated-at.bofh.it> |
| In reply to | #1591496 |
On Thu, Mar 02, 2017 at 01:44:39PM -0800, Laura Abbott wrote:
>
> Device specific platform support has been haphazard for Ion. There have
> been several independent attempts and there are still objections to
> what bindings exist right now. Just remove everything for a fresh start.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
It looks like with this we could remove a lot of the EXPORT_SYMBOL
statements from the ion code. Might be good to follow up with a patch to
clean those out.
Otherwise a patch that only removes code, what's not to love!
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/staging/android/ion/Kconfig | 35 ----
> drivers/staging/android/ion/Makefile | 6 -
> drivers/staging/android/ion/hisilicon/Kconfig | 5 -
> drivers/staging/android/ion/hisilicon/Makefile | 1 -
> drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 -------------
> drivers/staging/android/ion/ion_dummy_driver.c | 156 -----------------
> drivers/staging/android/ion/ion_of.c | 184 ---------------------
> drivers/staging/android/ion/ion_of.h | 37 -----
> drivers/staging/android/ion/tegra/Makefile | 1 -
> drivers/staging/android/ion/tegra/tegra_ion.c | 80 ---------
> 10 files changed, 618 deletions(-)
> delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig
> delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile
> delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c
> delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c
> delete mode 100644 drivers/staging/android/ion/ion_of.c
> delete mode 100644 drivers/staging/android/ion/ion_of.h
> delete mode 100644 drivers/staging/android/ion/tegra/Makefile
> delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c
>
> diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
> index c8fb413..0c91b2b 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -17,38 +17,3 @@ config ION_TEST
> Choose this option to create a device that can be used to test the
> kernel and device side ION functions.
>
> -config ION_DUMMY
> - bool "Dummy Ion driver"
> - depends on ION
> - help
> - Provides a dummy ION driver that registers the
> - /dev/ion device and some basic heaps. This can
> - be used for testing the ION infrastructure if
> - one doesn't have access to hardware drivers that
> - use ION.
> -
> -config ION_TEGRA
> - tristate "Ion for Tegra"
> - depends on ARCH_TEGRA && ION
> - help
> - Choose this option if you wish to use ion on an nVidia Tegra.
> -
> -config ION_HISI
> - tristate "Ion for Hisilicon"
> - depends on ARCH_HISI && ION
> - select ION_OF
> - help
> - Choose this option if you wish to use ion on Hisilicon Platform.
> -
> -source "drivers/staging/android/ion/hisilicon/Kconfig"
> -
> -config ION_OF
> - bool "Devicetree support for Ion"
> - depends on ION && OF_ADDRESS
> - help
> - Provides base support for defining Ion heaps in devicetree
> - and setting them up. Also includes functions for platforms
> - to parse the devicetree and expand for their own custom
> - extensions
> -
> - If using Ion and devicetree, you should say Y here
> diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
> index 5d630a0..9457090 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -5,9 +5,3 @@ obj-$(CONFIG_ION_TEST) += ion_test.o
> ifdef CONFIG_COMPAT
> obj-$(CONFIG_ION) += compat_ion.o
> endif
> -
> -obj-$(CONFIG_ION_DUMMY) += ion_dummy_driver.o
> -obj-$(CONFIG_ION_TEGRA) += tegra/
> -obj-$(CONFIG_ION_HISI) += hisilicon/
> -obj-$(CONFIG_ION_OF) += ion_of.o
> -
> diff --git a/drivers/staging/android/ion/hisilicon/Kconfig b/drivers/staging/android/ion/hisilicon/Kconfig
> deleted file mode 100644
> index 2b4bd07..0000000
> --- a/drivers/staging/android/ion/hisilicon/Kconfig
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -config HI6220_ION
> - bool "Hi6220 ION Driver"
> - depends on ARCH_HISI && ION
> - help
> - Build the Hisilicon Hi6220 ion driver.
> diff --git a/drivers/staging/android/ion/hisilicon/Makefile b/drivers/staging/android/ion/hisilicon/Makefile
> deleted file mode 100644
> index 2a89414..0000000
> --- a/drivers/staging/android/ion/hisilicon/Makefile
> +++ /dev/null
> @@ -1 +0,0 @@
> -obj-$(CONFIG_HI6220_ION) += hi6220_ion.o
> diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> deleted file mode 100644
> index 0de7897..0000000
> --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c
> +++ /dev/null
> @@ -1,113 +0,0 @@
> -/*
> - * Hisilicon Hi6220 ION Driver
> - *
> - * Copyright (c) 2015 Hisilicon Limited.
> - *
> - * Author: Chen Feng <puck.chen@hisilicon.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -
> -#define pr_fmt(fmt) "Ion: " fmt
> -
> -#include <linux/err.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> -#include <linux/of.h>
> -#include <linux/mm.h>
> -#include "../ion_priv.h"
> -#include "../ion.h"
> -#include "../ion_of.h"
> -
> -struct hisi_ion_dev {
> - struct ion_heap **heaps;
> - struct ion_device *idev;
> - struct ion_platform_data *data;
> -};
> -
> -static struct ion_of_heap hisi_heaps[] = {
> - PLATFORM_HEAP("hisilicon,sys_user", 0,
> - ION_HEAP_TYPE_SYSTEM, "sys_user"),
> - PLATFORM_HEAP("hisilicon,sys_contig", 1,
> - ION_HEAP_TYPE_SYSTEM_CONTIG, "sys_contig"),
> - PLATFORM_HEAP("hisilicon,cma", ION_HEAP_TYPE_DMA, ION_HEAP_TYPE_DMA,
> - "cma"),
> - {}
> -};
> -
> -static int hi6220_ion_probe(struct platform_device *pdev)
> -{
> - struct hisi_ion_dev *ipdev;
> - int i;
> -
> - ipdev = devm_kzalloc(&pdev->dev, sizeof(*ipdev), GFP_KERNEL);
> - if (!ipdev)
> - return -ENOMEM;
> -
> - platform_set_drvdata(pdev, ipdev);
> -
> - ipdev->idev = ion_device_create(NULL);
> - if (IS_ERR(ipdev->idev))
> - return PTR_ERR(ipdev->idev);
> -
> - ipdev->data = ion_parse_dt(pdev, hisi_heaps);
> - if (IS_ERR(ipdev->data))
> - return PTR_ERR(ipdev->data);
> -
> - ipdev->heaps = devm_kzalloc(&pdev->dev,
> - sizeof(struct ion_heap) * ipdev->data->nr,
> - GFP_KERNEL);
> - if (!ipdev->heaps) {
> - ion_destroy_platform_data(ipdev->data);
> - return -ENOMEM;
> - }
> -
> - for (i = 0; i < ipdev->data->nr; i++) {
> - ipdev->heaps[i] = ion_heap_create(&ipdev->data->heaps[i]);
> - if (!ipdev->heaps) {
> - ion_destroy_platform_data(ipdev->data);
> - return -ENOMEM;
> - }
> - ion_device_add_heap(ipdev->idev, ipdev->heaps[i]);
> - }
> - return 0;
> -}
> -
> -static int hi6220_ion_remove(struct platform_device *pdev)
> -{
> - struct hisi_ion_dev *ipdev;
> - int i;
> -
> - ipdev = platform_get_drvdata(pdev);
> -
> - for (i = 0; i < ipdev->data->nr; i++)
> - ion_heap_destroy(ipdev->heaps[i]);
> -
> - ion_destroy_platform_data(ipdev->data);
> - ion_device_destroy(ipdev->idev);
> -
> - return 0;
> -}
> -
> -static const struct of_device_id hi6220_ion_match_table[] = {
> - {.compatible = "hisilicon,hi6220-ion"},
> - {},
> -};
> -
> -static struct platform_driver hi6220_ion_driver = {
> - .probe = hi6220_ion_probe,
> - .remove = hi6220_ion_remove,
> - .driver = {
> - .name = "ion-hi6220",
> - .of_match_table = hi6220_ion_match_table,
> - },
> -};
> -
> -static int __init hi6220_ion_init(void)
> -{
> - return platform_driver_register(&hi6220_ion_driver);
> -}
> -
> -subsys_initcall(hi6220_ion_init);
> diff --git a/drivers/staging/android/ion/ion_dummy_driver.c b/drivers/staging/android/ion/ion_dummy_driver.c
> deleted file mode 100644
> index cf5c010..0000000
> --- a/drivers/staging/android/ion/ion_dummy_driver.c
> +++ /dev/null
> @@ -1,156 +0,0 @@
> -/*
> - * drivers/gpu/ion/ion_dummy_driver.c
> - *
> - * Copyright (C) 2013 Linaro, Inc
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - */
> -
> -#include <linux/err.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> -#include <linux/init.h>
> -#include <linux/bootmem.h>
> -#include <linux/memblock.h>
> -#include <linux/sizes.h>
> -#include <linux/io.h>
> -#include "ion.h"
> -#include "ion_priv.h"
> -
> -static struct ion_device *idev;
> -static struct ion_heap **heaps;
> -
> -static void *carveout_ptr;
> -static void *chunk_ptr;
> -
> -static struct ion_platform_heap dummy_heaps[] = {
> - {
> - .id = ION_HEAP_TYPE_SYSTEM,
> - .type = ION_HEAP_TYPE_SYSTEM,
> - .name = "system",
> - },
> - {
> - .id = ION_HEAP_TYPE_SYSTEM_CONTIG,
> - .type = ION_HEAP_TYPE_SYSTEM_CONTIG,
> - .name = "system contig",
> - },
> - {
> - .id = ION_HEAP_TYPE_CARVEOUT,
> - .type = ION_HEAP_TYPE_CARVEOUT,
> - .name = "carveout",
> - .size = SZ_4M,
> - },
> - {
> - .id = ION_HEAP_TYPE_CHUNK,
> - .type = ION_HEAP_TYPE_CHUNK,
> - .name = "chunk",
> - .size = SZ_4M,
> - .align = SZ_16K,
> - .priv = (void *)(SZ_16K),
> - },
> -};
> -
> -static const struct ion_platform_data dummy_ion_pdata = {
> - .nr = ARRAY_SIZE(dummy_heaps),
> - .heaps = dummy_heaps,
> -};
> -
> -static int __init ion_dummy_init(void)
> -{
> - int i, err;
> -
> - idev = ion_device_create(NULL);
> - if (IS_ERR(idev))
> - return PTR_ERR(idev);
> - heaps = kcalloc(dummy_ion_pdata.nr, sizeof(struct ion_heap *),
> - GFP_KERNEL);
> - if (!heaps)
> - return -ENOMEM;
> -
> -
> - /* Allocate a dummy carveout heap */
> - carveout_ptr = alloc_pages_exact(
> - dummy_heaps[ION_HEAP_TYPE_CARVEOUT].size,
> - GFP_KERNEL);
> - if (carveout_ptr)
> - dummy_heaps[ION_HEAP_TYPE_CARVEOUT].base =
> - virt_to_phys(carveout_ptr);
> - else
> - pr_err("ion_dummy: Could not allocate carveout\n");
> -
> - /* Allocate a dummy chunk heap */
> - chunk_ptr = alloc_pages_exact(
> - dummy_heaps[ION_HEAP_TYPE_CHUNK].size,
> - GFP_KERNEL);
> - if (chunk_ptr)
> - dummy_heaps[ION_HEAP_TYPE_CHUNK].base = virt_to_phys(chunk_ptr);
> - else
> - pr_err("ion_dummy: Could not allocate chunk\n");
> -
> - for (i = 0; i < dummy_ion_pdata.nr; i++) {
> - struct ion_platform_heap *heap_data = &dummy_ion_pdata.heaps[i];
> -
> - if (heap_data->type == ION_HEAP_TYPE_CARVEOUT &&
> - !heap_data->base)
> - continue;
> -
> - if (heap_data->type == ION_HEAP_TYPE_CHUNK && !heap_data->base)
> - continue;
> -
> - heaps[i] = ion_heap_create(heap_data);
> - if (IS_ERR_OR_NULL(heaps[i])) {
> - err = PTR_ERR(heaps[i]);
> - goto err;
> - }
> - ion_device_add_heap(idev, heaps[i]);
> - }
> - return 0;
> -err:
> - for (i = 0; i < dummy_ion_pdata.nr; ++i)
> - ion_heap_destroy(heaps[i]);
> - kfree(heaps);
> -
> - if (carveout_ptr) {
> - free_pages_exact(carveout_ptr,
> - dummy_heaps[ION_HEAP_TYPE_CARVEOUT].size);
> - carveout_ptr = NULL;
> - }
> - if (chunk_ptr) {
> - free_pages_exact(chunk_ptr,
> - dummy_heaps[ION_HEAP_TYPE_CHUNK].size);
> - chunk_ptr = NULL;
> - }
> - return err;
> -}
> -device_initcall(ion_dummy_init);
> -
> -static void __exit ion_dummy_exit(void)
> -{
> - int i;
> -
> - ion_device_destroy(idev);
> -
> - for (i = 0; i < dummy_ion_pdata.nr; i++)
> - ion_heap_destroy(heaps[i]);
> - kfree(heaps);
> -
> - if (carveout_ptr) {
> - free_pages_exact(carveout_ptr,
> - dummy_heaps[ION_HEAP_TYPE_CARVEOUT].size);
> - carveout_ptr = NULL;
> - }
> - if (chunk_ptr) {
> - free_pages_exact(chunk_ptr,
> - dummy_heaps[ION_HEAP_TYPE_CHUNK].size);
> - chunk_ptr = NULL;
> - }
> -}
> -__exitcall(ion_dummy_exit);
> diff --git a/drivers/staging/android/ion/ion_of.c b/drivers/staging/android/ion/ion_of.c
> deleted file mode 100644
> index 7791c70..0000000
> --- a/drivers/staging/android/ion/ion_of.c
> +++ /dev/null
> @@ -1,184 +0,0 @@
> -/*
> - * Based on work from:
> - * Andrew Andrianov <andrew@ncrmnt.org>
> - * Google
> - * The Linux Foundation
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -
> -#include <linux/init.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> -#include <linux/of.h>
> -#include <linux/of_platform.h>
> -#include <linux/of_address.h>
> -#include <linux/clk.h>
> -#include <linux/dma-mapping.h>
> -#include <linux/cma.h>
> -#include <linux/dma-contiguous.h>
> -#include <linux/io.h>
> -#include <linux/of_reserved_mem.h>
> -#include "ion.h"
> -#include "ion_priv.h"
> -#include "ion_of.h"
> -
> -static int ion_parse_dt_heap_common(struct device_node *heap_node,
> - struct ion_platform_heap *heap,
> - struct ion_of_heap *compatible)
> -{
> - int i;
> -
> - for (i = 0; compatible[i].name; i++) {
> - if (of_device_is_compatible(heap_node, compatible[i].compat))
> - break;
> - }
> -
> - if (!compatible[i].name)
> - return -ENODEV;
> -
> - heap->id = compatible[i].heap_id;
> - heap->type = compatible[i].type;
> - heap->name = compatible[i].name;
> - heap->align = compatible[i].align;
> -
> - /* Some kind of callback function pointer? */
> -
> - pr_info("%s: id %d type %d name %s align %lx\n", __func__,
> - heap->id, heap->type, heap->name, heap->align);
> - return 0;
> -}
> -
> -static int ion_setup_heap_common(struct platform_device *parent,
> - struct device_node *heap_node,
> - struct ion_platform_heap *heap)
> -{
> - int ret = 0;
> -
> - switch (heap->type) {
> - case ION_HEAP_TYPE_CARVEOUT:
> - case ION_HEAP_TYPE_CHUNK:
> - if (heap->base && heap->size)
> - return 0;
> -
> - ret = of_reserved_mem_device_init(heap->priv);
> - break;
> - default:
> - break;
> - }
> -
> - return ret;
> -}
> -
> -struct ion_platform_data *ion_parse_dt(struct platform_device *pdev,
> - struct ion_of_heap *compatible)
> -{
> - int num_heaps, ret;
> - const struct device_node *dt_node = pdev->dev.of_node;
> - struct device_node *node;
> - struct ion_platform_heap *heaps;
> - struct ion_platform_data *data;
> - int i = 0;
> -
> - num_heaps = of_get_available_child_count(dt_node);
> -
> - if (!num_heaps)
> - return ERR_PTR(-EINVAL);
> -
> - heaps = devm_kzalloc(&pdev->dev,
> - sizeof(struct ion_platform_heap) * num_heaps,
> - GFP_KERNEL);
> - if (!heaps)
> - return ERR_PTR(-ENOMEM);
> -
> - data = devm_kzalloc(&pdev->dev, sizeof(struct ion_platform_data),
> - GFP_KERNEL);
> - if (!data)
> - return ERR_PTR(-ENOMEM);
> -
> - for_each_available_child_of_node(dt_node, node) {
> - struct platform_device *heap_pdev;
> -
> - ret = ion_parse_dt_heap_common(node, &heaps[i], compatible);
> - if (ret)
> - return ERR_PTR(ret);
> -
> - heap_pdev = of_platform_device_create(node, heaps[i].name,
> - &pdev->dev);
> - if (!heap_pdev)
> - return ERR_PTR(-ENOMEM);
> - heap_pdev->dev.platform_data = &heaps[i];
> -
> - heaps[i].priv = &heap_pdev->dev;
> -
> - ret = ion_setup_heap_common(pdev, node, &heaps[i]);
> - if (ret)
> - goto out_err;
> - i++;
> - }
> -
> - data->heaps = heaps;
> - data->nr = num_heaps;
> - return data;
> -
> -out_err:
> - for ( ; i >= 0; i--)
> - if (heaps[i].priv)
> - of_device_unregister(to_platform_device(heaps[i].priv));
> -
> - return ERR_PTR(ret);
> -}
> -
> -void ion_destroy_platform_data(struct ion_platform_data *data)
> -{
> - int i;
> -
> - for (i = 0; i < data->nr; i++)
> - if (data->heaps[i].priv)
> - of_device_unregister(to_platform_device(
> - data->heaps[i].priv));
> -}
> -
> -#ifdef CONFIG_OF_RESERVED_MEM
> -#include <linux/of.h>
> -#include <linux/of_fdt.h>
> -#include <linux/of_reserved_mem.h>
> -
> -static int rmem_ion_device_init(struct reserved_mem *rmem, struct device *dev)
> -{
> - struct platform_device *pdev = to_platform_device(dev);
> - struct ion_platform_heap *heap = pdev->dev.platform_data;
> -
> - heap->base = rmem->base;
> - heap->base = rmem->size;
> - pr_debug("%s: heap %s base %pa size %pa dev %p\n", __func__,
> - heap->name, &rmem->base, &rmem->size, dev);
> - return 0;
> -}
> -
> -static void rmem_ion_device_release(struct reserved_mem *rmem,
> - struct device *dev)
> -{
> -}
> -
> -static const struct reserved_mem_ops rmem_dma_ops = {
> - .device_init = rmem_ion_device_init,
> - .device_release = rmem_ion_device_release,
> -};
> -
> -static int __init rmem_ion_setup(struct reserved_mem *rmem)
> -{
> - phys_addr_t size = rmem->size;
> -
> - size = size / 1024;
> -
> - pr_info("Ion memory setup at %pa size %pa MiB\n",
> - &rmem->base, &size);
> - rmem->ops = &rmem_dma_ops;
> - return 0;
> -}
> -
> -RESERVEDMEM_OF_DECLARE(ion, "ion-region", rmem_ion_setup);
> -#endif
> diff --git a/drivers/staging/android/ion/ion_of.h b/drivers/staging/android/ion/ion_of.h
> deleted file mode 100644
> index 8241a17..0000000
> --- a/drivers/staging/android/ion/ion_of.h
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -/*
> - * Based on work from:
> - * Andrew Andrianov <andrew@ncrmnt.org>
> - * Google
> - * The Linux Foundation
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -
> -#ifndef _ION_OF_H
> -#define _ION_OF_H
> -
> -struct ion_of_heap {
> - const char *compat;
> - int heap_id;
> - int type;
> - const char *name;
> - int align;
> -};
> -
> -#define PLATFORM_HEAP(_compat, _id, _type, _name) \
> -{ \
> - .compat = _compat, \
> - .heap_id = _id, \
> - .type = _type, \
> - .name = _name, \
> - .align = PAGE_SIZE, \
> -}
> -
> -struct ion_platform_data *ion_parse_dt(struct platform_device *pdev,
> - struct ion_of_heap *compatible);
> -
> -void ion_destroy_platform_data(struct ion_platform_data *data);
> -
> -#endif
> diff --git a/drivers/staging/android/ion/tegra/Makefile b/drivers/staging/android/ion/tegra/Makefile
> deleted file mode 100644
> index 808f1f5..0000000
> --- a/drivers/staging/android/ion/tegra/Makefile
> +++ /dev/null
> @@ -1 +0,0 @@
> -obj-$(CONFIG_ION_TEGRA) += tegra_ion.o
> diff --git a/drivers/staging/android/ion/tegra/tegra_ion.c b/drivers/staging/android/ion/tegra/tegra_ion.c
> deleted file mode 100644
> index 49e55e5..0000000
> --- a/drivers/staging/android/ion/tegra/tegra_ion.c
> +++ /dev/null
> @@ -1,80 +0,0 @@
> -/*
> - * drivers/gpu/tegra/tegra_ion.c
> - *
> - * Copyright (C) 2011 Google, Inc.
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - */
> -
> -#include <linux/err.h>
> -#include <linux/module.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> -#include "../ion.h"
> -#include "../ion_priv.h"
> -
> -static struct ion_device *idev;
> -static int num_heaps;
> -static struct ion_heap **heaps;
> -
> -static int tegra_ion_probe(struct platform_device *pdev)
> -{
> - struct ion_platform_data *pdata = pdev->dev.platform_data;
> - int err;
> - int i;
> -
> - num_heaps = pdata->nr;
> -
> - heaps = devm_kcalloc(&pdev->dev, pdata->nr,
> - sizeof(struct ion_heap *), GFP_KERNEL);
> -
> - idev = ion_device_create(NULL);
> - if (IS_ERR(idev))
> - return PTR_ERR(idev);
> -
> - /* create the heaps as specified in the board file */
> - for (i = 0; i < num_heaps; i++) {
> - struct ion_platform_heap *heap_data = &pdata->heaps[i];
> -
> - heaps[i] = ion_heap_create(heap_data);
> - if (IS_ERR_OR_NULL(heaps[i])) {
> - err = PTR_ERR(heaps[i]);
> - goto err;
> - }
> - ion_device_add_heap(idev, heaps[i]);
> - }
> - platform_set_drvdata(pdev, idev);
> - return 0;
> -err:
> - for (i = 0; i < num_heaps; ++i)
> - ion_heap_destroy(heaps[i]);
> - return err;
> -}
> -
> -static int tegra_ion_remove(struct platform_device *pdev)
> -{
> - struct ion_device *idev = platform_get_drvdata(pdev);
> - int i;
> -
> - ion_device_destroy(idev);
> - for (i = 0; i < num_heaps; i++)
> - ion_heap_destroy(heaps[i]);
> - return 0;
> -}
> -
> -static struct platform_driver ion_driver = {
> - .probe = tegra_ion_probe,
> - .remove = tegra_ion_remove,
> - .driver = { .name = "ion-tegra" }
> -};
> -
> -module_platform_driver(ion_driver);
> -
> --
> 2.7.4
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2017-03-02 22:50 +0100 |
| Subject | [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <tgGLN-8cb-39@gated-at.bofh.it> |
| In reply to | #1591485 |
Now that we call dma_map in the dma_buf API callbacks there is no need
to use the existing cache APIs. Remove the sync ioctl and the existing
bad dma_sync calls. Explicit caching can be handled with the dma_buf
sync API.
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
drivers/staging/android/ion/ion-ioctl.c | 5 ----
drivers/staging/android/ion/ion.c | 40 -------------------------
drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
drivers/staging/android/ion/ion_page_pool.c | 3 --
drivers/staging/android/ion/ion_system_heap.c | 5 ----
6 files changed, 65 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 5b2e93f..f820d77 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
data.handle.handle = handle->id;
break;
}
- case ION_IOC_SYNC:
- {
- ret = ion_sync_for_device(client, data.fd.fd);
- break;
- }
case ION_IOC_CUSTOM:
{
if (!dev->custom_ioctl)
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 8eef1d7..c3c316f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -815,22 +815,6 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
free_duped_table(table);
}
-void ion_pages_sync_for_device(struct device *dev, struct page *page,
- size_t size, enum dma_data_direction dir)
-{
- struct scatterlist sg;
-
- sg_init_table(&sg, 1);
- sg_set_page(&sg, page, size, 0);
- /*
- * This is not correct - sg_dma_address needs a dma_addr_t that is valid
- * for the targeted device, but this works on the currently targeted
- * hardware.
- */
- sg_dma_address(&sg) = page_to_phys(page);
- dma_sync_sg_for_device(dev, &sg, 1, dir);
-}
-
static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
{
struct ion_buffer *buffer = dmabuf->priv;
@@ -1042,30 +1026,6 @@ struct ion_handle *ion_import_dma_buf_fd(struct ion_client *client, int fd)
}
EXPORT_SYMBOL(ion_import_dma_buf_fd);
-int ion_sync_for_device(struct ion_client *client, int fd)
-{
- struct dma_buf *dmabuf;
- struct ion_buffer *buffer;
-
- dmabuf = dma_buf_get(fd);
- if (IS_ERR(dmabuf))
- return PTR_ERR(dmabuf);
-
- /* if this memory came from ion */
- if (dmabuf->ops != &dma_buf_ops) {
- pr_err("%s: can not sync dmabuf from another exporter\n",
- __func__);
- dma_buf_put(dmabuf);
- return -EINVAL;
- }
- buffer = dmabuf->priv;
-
- dma_sync_sg_for_device(NULL, buffer->sg_table->sgl,
- buffer->sg_table->nents, DMA_BIDIRECTIONAL);
- dma_buf_put(dmabuf);
- return 0;
-}
-
int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
{
struct ion_device *dev = client->dev;
diff --git a/drivers/staging/android/ion/ion_carveout_heap.c b/drivers/staging/android/ion/ion_carveout_heap.c
index 9bf8e98..e0e360f 100644
--- a/drivers/staging/android/ion/ion_carveout_heap.c
+++ b/drivers/staging/android/ion/ion_carveout_heap.c
@@ -100,10 +100,6 @@ static void ion_carveout_heap_free(struct ion_buffer *buffer)
ion_heap_buffer_zero(buffer);
- if (ion_buffer_cached(buffer))
- dma_sync_sg_for_device(NULL, table->sgl, table->nents,
- DMA_BIDIRECTIONAL);
-
ion_carveout_free(heap, paddr, buffer->size);
sg_free_table(table);
kfree(table);
@@ -128,8 +124,6 @@ struct ion_heap *ion_carveout_heap_create(struct ion_platform_heap *heap_data)
page = pfn_to_page(PFN_DOWN(heap_data->base));
size = heap_data->size;
- ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
-
ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c b/drivers/staging/android/ion/ion_chunk_heap.c
index 8c41889..46e13f6 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -101,10 +101,6 @@ static void ion_chunk_heap_free(struct ion_buffer *buffer)
ion_heap_buffer_zero(buffer);
- if (ion_buffer_cached(buffer))
- dma_sync_sg_for_device(NULL, table->sgl, table->nents,
- DMA_BIDIRECTIONAL);
-
for_each_sg(table->sgl, sg, table->nents, i) {
gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)),
sg->length);
@@ -132,8 +128,6 @@ struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap *heap_data)
page = pfn_to_page(PFN_DOWN(heap_data->base));
size = heap_data->size;
- ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
-
ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index aea89c1..532eda7 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -30,9 +30,6 @@ static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool)
if (!page)
return NULL;
- if (!pool->cached)
- ion_pages_sync_for_device(NULL, page, PAGE_SIZE << pool->order,
- DMA_BIDIRECTIONAL);
return page;
}
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index 6cb2fe7..a33331b 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -75,9 +75,6 @@ static struct page *alloc_buffer_page(struct ion_system_heap *heap,
page = ion_page_pool_alloc(pool);
- if (cached)
- ion_pages_sync_for_device(NULL, page, PAGE_SIZE << order,
- DMA_BIDIRECTIONAL);
return page;
}
@@ -401,8 +398,6 @@ static int ion_system_contig_heap_allocate(struct ion_heap *heap,
buffer->sg_table = table;
- ion_pages_sync_for_device(NULL, page, len, DMA_BIDIRECTIONAL);
-
return 0;
free_table:
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-03 11:00 +0100 |
| Subject | Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <tgSae-7JR-11@gated-at.bofh.it> |
| In reply to | #1591497 |
On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
>
>
> Now that we call dma_map in the dma_buf API callbacks there is no need
> to use the existing cache APIs. Remove the sync ioctl and the existing
> bad dma_sync calls. Explicit caching can be handled with the dma_buf
> sync API.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> drivers/staging/android/ion/ion-ioctl.c | 5 ----
> drivers/staging/android/ion/ion.c | 40 -------------------------
> drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
> drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
> drivers/staging/android/ion/ion_page_pool.c | 3 --
> drivers/staging/android/ion/ion_system_heap.c | 5 ----
> 6 files changed, 65 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index 5b2e93f..f820d77 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> data.handle.handle = handle->id;
> break;
> }
> - case ION_IOC_SYNC:
> - {
> - ret = ion_sync_for_device(client, data.fd.fd);
> - break;
> - }
You missed the case ION_IOC_SYNC: in compat_ion.c.
While at it: Should we also remove the entire custom_ioctl infrastructure?
It's entirely unused afaict, and for a pure buffer allocator I don't see
any need to have custom ioctl.
More code to remove potentially:
- The entire compat ioctl stuff - would be an abi break, but I guess if we
pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
would allow us to remove compat_ion.c entirely.
- ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
still need to be able to import anything. All the cache flushing/mapping
is done through dma-buf ops/ioctls.
With the case in compat_ion.c also removed, this patch is:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> case ION_IOC_CUSTOM:
> {
> if (!dev->custom_ioctl)
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 8eef1d7..c3c316f 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -815,22 +815,6 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
> free_duped_table(table);
> }
>
> -void ion_pages_sync_for_device(struct device *dev, struct page *page,
> - size_t size, enum dma_data_direction dir)
> -{
> - struct scatterlist sg;
> -
> - sg_init_table(&sg, 1);
> - sg_set_page(&sg, page, size, 0);
> - /*
> - * This is not correct - sg_dma_address needs a dma_addr_t that is valid
> - * for the targeted device, but this works on the currently targeted
> - * hardware.
> - */
> - sg_dma_address(&sg) = page_to_phys(page);
> - dma_sync_sg_for_device(dev, &sg, 1, dir);
> -}
> -
> static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> {
> struct ion_buffer *buffer = dmabuf->priv;
> @@ -1042,30 +1026,6 @@ struct ion_handle *ion_import_dma_buf_fd(struct ion_client *client, int fd)
> }
> EXPORT_SYMBOL(ion_import_dma_buf_fd);
>
> -int ion_sync_for_device(struct ion_client *client, int fd)
> -{
> - struct dma_buf *dmabuf;
> - struct ion_buffer *buffer;
> -
> - dmabuf = dma_buf_get(fd);
> - if (IS_ERR(dmabuf))
> - return PTR_ERR(dmabuf);
> -
> - /* if this memory came from ion */
> - if (dmabuf->ops != &dma_buf_ops) {
> - pr_err("%s: can not sync dmabuf from another exporter\n",
> - __func__);
> - dma_buf_put(dmabuf);
> - return -EINVAL;
> - }
> - buffer = dmabuf->priv;
> -
> - dma_sync_sg_for_device(NULL, buffer->sg_table->sgl,
> - buffer->sg_table->nents, DMA_BIDIRECTIONAL);
> - dma_buf_put(dmabuf);
> - return 0;
> -}
> -
> int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query)
> {
> struct ion_device *dev = client->dev;
> diff --git a/drivers/staging/android/ion/ion_carveout_heap.c b/drivers/staging/android/ion/ion_carveout_heap.c
> index 9bf8e98..e0e360f 100644
> --- a/drivers/staging/android/ion/ion_carveout_heap.c
> +++ b/drivers/staging/android/ion/ion_carveout_heap.c
> @@ -100,10 +100,6 @@ static void ion_carveout_heap_free(struct ion_buffer *buffer)
>
> ion_heap_buffer_zero(buffer);
>
> - if (ion_buffer_cached(buffer))
> - dma_sync_sg_for_device(NULL, table->sgl, table->nents,
> - DMA_BIDIRECTIONAL);
> -
> ion_carveout_free(heap, paddr, buffer->size);
> sg_free_table(table);
> kfree(table);
> @@ -128,8 +124,6 @@ struct ion_heap *ion_carveout_heap_create(struct ion_platform_heap *heap_data)
> page = pfn_to_page(PFN_DOWN(heap_data->base));
> size = heap_data->size;
>
> - ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
> -
> ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
> if (ret)
> return ERR_PTR(ret);
> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c b/drivers/staging/android/ion/ion_chunk_heap.c
> index 8c41889..46e13f6 100644
> --- a/drivers/staging/android/ion/ion_chunk_heap.c
> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
> @@ -101,10 +101,6 @@ static void ion_chunk_heap_free(struct ion_buffer *buffer)
>
> ion_heap_buffer_zero(buffer);
>
> - if (ion_buffer_cached(buffer))
> - dma_sync_sg_for_device(NULL, table->sgl, table->nents,
> - DMA_BIDIRECTIONAL);
> -
> for_each_sg(table->sgl, sg, table->nents, i) {
> gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)),
> sg->length);
> @@ -132,8 +128,6 @@ struct ion_heap *ion_chunk_heap_create(struct ion_platform_heap *heap_data)
> page = pfn_to_page(PFN_DOWN(heap_data->base));
> size = heap_data->size;
>
> - ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
> -
> ret = ion_heap_pages_zero(page, size, pgprot_writecombine(PAGE_KERNEL));
> if (ret)
> return ERR_PTR(ret);
> diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
> index aea89c1..532eda7 100644
> --- a/drivers/staging/android/ion/ion_page_pool.c
> +++ b/drivers/staging/android/ion/ion_page_pool.c
> @@ -30,9 +30,6 @@ static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool)
>
> if (!page)
> return NULL;
> - if (!pool->cached)
> - ion_pages_sync_for_device(NULL, page, PAGE_SIZE << pool->order,
> - DMA_BIDIRECTIONAL);
> return page;
> }
>
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index 6cb2fe7..a33331b 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -75,9 +75,6 @@ static struct page *alloc_buffer_page(struct ion_system_heap *heap,
>
> page = ion_page_pool_alloc(pool);
>
> - if (cached)
> - ion_pages_sync_for_device(NULL, page, PAGE_SIZE << order,
> - DMA_BIDIRECTIONAL);
> return page;
> }
>
> @@ -401,8 +398,6 @@ static int ion_system_contig_heap_allocate(struct ion_heap *heap,
>
> buffer->sg_table = table;
>
> - ion_pages_sync_for_device(NULL, page, len, DMA_BIDIRECTIONAL);
> -
> return 0;
>
> free_table:
> --
> 2.7.4
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Laurent Pinchart <laurent.pinchart@ideasonboard.com> |
|---|---|
| Date | 2017-03-03 18:00 +0100 |
| Subject | Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <tgYIF-3UX-9@gated-at.bofh.it> |
| In reply to | #1591811 |
Hi Daniel,
On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:
> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
> > Now that we call dma_map in the dma_buf API callbacks there is no need
> > to use the existing cache APIs. Remove the sync ioctl and the existing
> > bad dma_sync calls. Explicit caching can be handled with the dma_buf
> > sync API.
> >
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > ---
> >
> > drivers/staging/android/ion/ion-ioctl.c | 5 ----
> > drivers/staging/android/ion/ion.c | 40 --------------------
> > drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
> > drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
> > drivers/staging/android/ion/ion_page_pool.c | 3 --
> > drivers/staging/android/ion/ion_system_heap.c | 5 ----
> > 6 files changed, 65 deletions(-)
> >
> > diff --git a/drivers/staging/android/ion/ion-ioctl.c
> > b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644
> > --- a/drivers/staging/android/ion/ion-ioctl.c
> > +++ b/drivers/staging/android/ion/ion-ioctl.c
> > @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
> > unsigned long arg)>
> > data.handle.handle = handle->id;
> >
> > break;
> >
> > }
> >
> > - case ION_IOC_SYNC:
> > - {
> > - ret = ion_sync_for_device(client, data.fd.fd);
> > - break;
> > - }
>
> You missed the case ION_IOC_SYNC: in compat_ion.c.
>
> While at it: Should we also remove the entire custom_ioctl infrastructure?
> It's entirely unused afaict, and for a pure buffer allocator I don't see
> any need to have custom ioctl.
I second that, if you want to make ion a standard API, then we certainly don't
want any custom ioctl.
> More code to remove potentially:
> - The entire compat ioctl stuff - would be an abi break, but I guess if we
> pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
> would allow us to remove compat_ion.c entirely.
>
> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
> still need to be able to import anything. All the cache flushing/mapping
> is done through dma-buf ops/ioctls.
>
>
> With the case in compat_ion.c also removed, this patch is:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> > case ION_IOC_CUSTOM:
> > {
> >
> > if (!dev->custom_ioctl)
> >
> > diff --git a/drivers/staging/android/ion/ion.c
> > b/drivers/staging/android/ion/ion.c index 8eef1d7..c3c316f 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -815,22 +815,6 @@ static void ion_unmap_dma_buf(struct
> > dma_buf_attachment *attachment,>
> > free_duped_table(table);
> >
> > }
> >
> > -void ion_pages_sync_for_device(struct device *dev, struct page *page,
> > - size_t size, enum dma_data_direction dir)
> > -{
> > - struct scatterlist sg;
> > -
> > - sg_init_table(&sg, 1);
> > - sg_set_page(&sg, page, size, 0);
> > - /*
> > - * This is not correct - sg_dma_address needs a dma_addr_t that is
valid
> > - * for the targeted device, but this works on the currently targeted
> > - * hardware.
> > - */
> > - sg_dma_address(&sg) = page_to_phys(page);
> > - dma_sync_sg_for_device(dev, &sg, 1, dir);
> > -}
> > -
> >
> > static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> > {
> >
> > struct ion_buffer *buffer = dmabuf->priv;
> >
> > @@ -1042,30 +1026,6 @@ struct ion_handle *ion_import_dma_buf_fd(struct
> > ion_client *client, int fd)>
> > }
> > EXPORT_SYMBOL(ion_import_dma_buf_fd);
> >
> > -int ion_sync_for_device(struct ion_client *client, int fd)
> > -{
> > - struct dma_buf *dmabuf;
> > - struct ion_buffer *buffer;
> > -
> > - dmabuf = dma_buf_get(fd);
> > - if (IS_ERR(dmabuf))
> > - return PTR_ERR(dmabuf);
> > -
> > - /* if this memory came from ion */
> > - if (dmabuf->ops != &dma_buf_ops) {
> > - pr_err("%s: can not sync dmabuf from another exporter\n",
> > - __func__);
> > - dma_buf_put(dmabuf);
> > - return -EINVAL;
> > - }
> > - buffer = dmabuf->priv;
> > -
> > - dma_sync_sg_for_device(NULL, buffer->sg_table->sgl,
> > - buffer->sg_table->nents, DMA_BIDIRECTIONAL);
> > - dma_buf_put(dmabuf);
> > - return 0;
> > -}
> > -
> >
> > int ion_query_heaps(struct ion_client *client, struct ion_heap_query
> > *query) {
> >
> > struct ion_device *dev = client->dev;
> >
> > diff --git a/drivers/staging/android/ion/ion_carveout_heap.c
> > b/drivers/staging/android/ion/ion_carveout_heap.c index 9bf8e98..e0e360f
> > 100644
> > --- a/drivers/staging/android/ion/ion_carveout_heap.c
> > +++ b/drivers/staging/android/ion/ion_carveout_heap.c
> > @@ -100,10 +100,6 @@ static void ion_carveout_heap_free(struct ion_buffer
> > *buffer)>
> > ion_heap_buffer_zero(buffer);
> >
> > - if (ion_buffer_cached(buffer))
> > - dma_sync_sg_for_device(NULL, table->sgl, table->nents,
> > - DMA_BIDIRECTIONAL);
> > -
> >
> > ion_carveout_free(heap, paddr, buffer->size);
> > sg_free_table(table);
> > kfree(table);
> >
> > @@ -128,8 +124,6 @@ struct ion_heap *ion_carveout_heap_create(struct
> > ion_platform_heap *heap_data)>
> > page = pfn_to_page(PFN_DOWN(heap_data->base));
> > size = heap_data->size;
> >
> > - ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
> > -
> >
> > ret = ion_heap_pages_zero(page, size,
pgprot_writecombine(PAGE_KERNEL));
> > if (ret)
> >
> > return ERR_PTR(ret);
> >
> > diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
> > b/drivers/staging/android/ion/ion_chunk_heap.c index 8c41889..46e13f6
> > 100644
> > --- a/drivers/staging/android/ion/ion_chunk_heap.c
> > +++ b/drivers/staging/android/ion/ion_chunk_heap.c
> > @@ -101,10 +101,6 @@ static void ion_chunk_heap_free(struct ion_buffer
> > *buffer)>
> > ion_heap_buffer_zero(buffer);
> >
> > - if (ion_buffer_cached(buffer))
> > - dma_sync_sg_for_device(NULL, table->sgl, table->nents,
> > - DMA_BIDIRECTIONAL);
> > -
> >
> > for_each_sg(table->sgl, sg, table->nents, i) {
> >
> > gen_pool_free(chunk_heap->pool, page_to_phys(sg_page(sg)),
> >
> > sg->length);
> >
> > @@ -132,8 +128,6 @@ struct ion_heap *ion_chunk_heap_create(struct
> > ion_platform_heap *heap_data)>
> > page = pfn_to_page(PFN_DOWN(heap_data->base));
> > size = heap_data->size;
> >
> > - ion_pages_sync_for_device(NULL, page, size, DMA_BIDIRECTIONAL);
> > -
> >
> > ret = ion_heap_pages_zero(page, size,
pgprot_writecombine(PAGE_KERNEL));
> > if (ret)
> >
> > return ERR_PTR(ret);
> >
> > diff --git a/drivers/staging/android/ion/ion_page_pool.c
> > b/drivers/staging/android/ion/ion_page_pool.c index aea89c1..532eda7
> > 100644
> > --- a/drivers/staging/android/ion/ion_page_pool.c
> > +++ b/drivers/staging/android/ion/ion_page_pool.c
> > @@ -30,9 +30,6 @@ static void *ion_page_pool_alloc_pages(struct
> > ion_page_pool *pool)>
> > if (!page)
> >
> > return NULL;
> >
> > - if (!pool->cached)
> > - ion_pages_sync_for_device(NULL, page, PAGE_SIZE << pool-
>order,
> > - DMA_BIDIRECTIONAL);
> >
> > return page;
> >
> > }
> >
> > diff --git a/drivers/staging/android/ion/ion_system_heap.c
> > b/drivers/staging/android/ion/ion_system_heap.c index 6cb2fe7..a33331b
> > 100644
> > --- a/drivers/staging/android/ion/ion_system_heap.c
> > +++ b/drivers/staging/android/ion/ion_system_heap.c
> > @@ -75,9 +75,6 @@ static struct page *alloc_buffer_page(struct
> > ion_system_heap *heap,>
> > page = ion_page_pool_alloc(pool);
> >
> > - if (cached)
> > - ion_pages_sync_for_device(NULL, page, PAGE_SIZE << order,
> > - DMA_BIDIRECTIONAL);
> >
> > return page;
> >
> > }
> >
> > @@ -401,8 +398,6 @@ static int ion_system_contig_heap_allocate(struct
> > ion_heap *heap,>
> > buffer->sg_table = table;
> >
> > - ion_pages_sync_for_device(NULL, page, len, DMA_BIDIRECTIONAL);
> > -
> >
> > return 0;
> >
> > free_table:
--
Regards,
Laurent Pinchart
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2017-03-03 19:50 +0100 |
| Subject | Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <th0r8-5c2-29@gated-at.bofh.it> |
| In reply to | #1592136 |
On 03/03/2017 08:39 AM, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:
>> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
>>> Now that we call dma_map in the dma_buf API callbacks there is no need
>>> to use the existing cache APIs. Remove the sync ioctl and the existing
>>> bad dma_sync calls. Explicit caching can be handled with the dma_buf
>>> sync API.
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>>
>>> drivers/staging/android/ion/ion-ioctl.c | 5 ----
>>> drivers/staging/android/ion/ion.c | 40 --------------------
>>> drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
>>> drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
>>> drivers/staging/android/ion/ion_page_pool.c | 3 --
>>> drivers/staging/android/ion/ion_system_heap.c | 5 ----
>>> 6 files changed, 65 deletions(-)
>>>
>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>> b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644
>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>>> unsigned long arg)>
>>> data.handle.handle = handle->id;
>>>
>>> break;
>>>
>>> }
>>>
>>> - case ION_IOC_SYNC:
>>> - {
>>> - ret = ion_sync_for_device(client, data.fd.fd);
>>> - break;
>>> - }
>>
>> You missed the case ION_IOC_SYNC: in compat_ion.c.
>>
>> While at it: Should we also remove the entire custom_ioctl infrastructure?
>> It's entirely unused afaict, and for a pure buffer allocator I don't see
>> any need to have custom ioctl.
>
> I second that, if you want to make ion a standard API, then we certainly don't
> want any custom ioctl.
>
>> More code to remove potentially:
>> - The entire compat ioctl stuff - would be an abi break, but I guess if we
>> pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
>> would allow us to remove compat_ion.c entirely.
>>
>> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
>> still need to be able to import anything. All the cache flushing/mapping
>> is done through dma-buf ops/ioctls.
>>
>>
Good point to all of the above. I was considering keeping the import around
for backwards compatibility reasons but given how much other stuff is being
potentially broken, everything should just get ripped out.
Thanks,
Laura
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-06 11:50 +0100 |
| Subject | Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <thYnf-6U9-9@gated-at.bofh.it> |
| In reply to | #1592215 |
On Fri, Mar 03, 2017 at 10:46:03AM -0800, Laura Abbott wrote:
> On 03/03/2017 08:39 AM, Laurent Pinchart wrote:
> > Hi Daniel,
> >
> > On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:
> >> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
> >>> Now that we call dma_map in the dma_buf API callbacks there is no need
> >>> to use the existing cache APIs. Remove the sync ioctl and the existing
> >>> bad dma_sync calls. Explicit caching can be handled with the dma_buf
> >>> sync API.
> >>>
> >>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>> ---
> >>>
> >>> drivers/staging/android/ion/ion-ioctl.c | 5 ----
> >>> drivers/staging/android/ion/ion.c | 40 --------------------
> >>> drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
> >>> drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
> >>> drivers/staging/android/ion/ion_page_pool.c | 3 --
> >>> drivers/staging/android/ion/ion_system_heap.c | 5 ----
> >>> 6 files changed, 65 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
> >>> b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644
> >>> --- a/drivers/staging/android/ion/ion-ioctl.c
> >>> +++ b/drivers/staging/android/ion/ion-ioctl.c
> >>> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
> >>> unsigned long arg)>
> >>> data.handle.handle = handle->id;
> >>>
> >>> break;
> >>>
> >>> }
> >>>
> >>> - case ION_IOC_SYNC:
> >>> - {
> >>> - ret = ion_sync_for_device(client, data.fd.fd);
> >>> - break;
> >>> - }
> >>
> >> You missed the case ION_IOC_SYNC: in compat_ion.c.
> >>
> >> While at it: Should we also remove the entire custom_ioctl infrastructure?
> >> It's entirely unused afaict, and for a pure buffer allocator I don't see
> >> any need to have custom ioctl.
> >
> > I second that, if you want to make ion a standard API, then we certainly don't
> > want any custom ioctl.
> >
> >> More code to remove potentially:
> >> - The entire compat ioctl stuff - would be an abi break, but I guess if we
> >> pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
> >> would allow us to remove compat_ion.c entirely.
> >>
> >> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
> >> still need to be able to import anything. All the cache flushing/mapping
> >> is done through dma-buf ops/ioctls.
> >>
> >>
>
> Good point to all of the above. I was considering keeping the import around
> for backwards compatibility reasons but given how much other stuff is being
> potentially broken, everything should just get ripped out.
If you're ok with breaking the world, then I strongly suggest we go
through the uapi header and replace all types with the standard
fixed-width ones (__s32, __s64 and __u32, __u64). Allows us to remove all
the compat ioctl code :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| Date | 2017-03-06 18:40 +0100 |
| Subject | Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <ti4M2-2ZV-15@gated-at.bofh.it> |
| In reply to | #1593177 |
On 6 March 2017 at 10:29, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 03, 2017 at 10:46:03AM -0800, Laura Abbott wrote:
>> On 03/03/2017 08:39 AM, Laurent Pinchart wrote:
>> > Hi Daniel,
>> >
>> > On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:
>> >> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
>> >>> Now that we call dma_map in the dma_buf API callbacks there is no need
>> >>> to use the existing cache APIs. Remove the sync ioctl and the existing
>> >>> bad dma_sync calls. Explicit caching can be handled with the dma_buf
>> >>> sync API.
>> >>>
>> >>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> >>> ---
>> >>>
>> >>> drivers/staging/android/ion/ion-ioctl.c | 5 ----
>> >>> drivers/staging/android/ion/ion.c | 40 --------------------
>> >>> drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
>> >>> drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
>> >>> drivers/staging/android/ion/ion_page_pool.c | 3 --
>> >>> drivers/staging/android/ion/ion_system_heap.c | 5 ----
>> >>> 6 files changed, 65 deletions(-)
>> >>>
>> >>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> >>> b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644
>> >>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> >>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> >>> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> >>> unsigned long arg)>
>> >>> data.handle.handle = handle->id;
>> >>>
>> >>> break;
>> >>>
>> >>> }
>> >>>
>> >>> - case ION_IOC_SYNC:
>> >>> - {
>> >>> - ret = ion_sync_for_device(client, data.fd.fd);
>> >>> - break;
>> >>> - }
>> >>
>> >> You missed the case ION_IOC_SYNC: in compat_ion.c.
>> >>
>> >> While at it: Should we also remove the entire custom_ioctl infrastructure?
>> >> It's entirely unused afaict, and for a pure buffer allocator I don't see
>> >> any need to have custom ioctl.
>> >
>> > I second that, if you want to make ion a standard API, then we certainly don't
>> > want any custom ioctl.
>> >
>> >> More code to remove potentially:
>> >> - The entire compat ioctl stuff - would be an abi break, but I guess if we
>> >> pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
>> >> would allow us to remove compat_ion.c entirely.
>> >>
>> >> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
>> >> still need to be able to import anything. All the cache flushing/mapping
>> >> is done through dma-buf ops/ioctls.
>> >>
>> >>
>>
>> Good point to all of the above. I was considering keeping the import around
>> for backwards compatibility reasons but given how much other stuff is being
>> potentially broken, everything should just get ripped out.
>
> If you're ok with breaking the world, then I strongly suggest we go
> through the uapi header and replace all types with the standard
> fixed-width ones (__s32, __s64 and __u32, __u64). Allows us to remove all
> the compat ioctl code :-)
I think the other comments from your "botching-up ioctls" [1] also apply ;-)
Namely - align structs to multiple of 64bit, add "flags" and properly
verity user input returning -EINVAL.
-Emil
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2017-03-06 20:30 +0100 |
| Subject | Re: [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support |
| Message-ID | <ti6ut-4kC-9@gated-at.bofh.it> |
| In reply to | #1593547 |
On 03/06/2017 09:00 AM, Emil Velikov wrote:
> On 6 March 2017 at 10:29, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Mar 03, 2017 at 10:46:03AM -0800, Laura Abbott wrote:
>>> On 03/03/2017 08:39 AM, Laurent Pinchart wrote:
>>>> Hi Daniel,
>>>>
>>>> On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:
>>>>> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:
>>>>>> Now that we call dma_map in the dma_buf API callbacks there is no need
>>>>>> to use the existing cache APIs. Remove the sync ioctl and the existing
>>>>>> bad dma_sync calls. Explicit caching can be handled with the dma_buf
>>>>>> sync API.
>>>>>>
>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>>>> ---
>>>>>>
>>>>>> drivers/staging/android/ion/ion-ioctl.c | 5 ----
>>>>>> drivers/staging/android/ion/ion.c | 40 --------------------
>>>>>> drivers/staging/android/ion/ion_carveout_heap.c | 6 ----
>>>>>> drivers/staging/android/ion/ion_chunk_heap.c | 6 ----
>>>>>> drivers/staging/android/ion/ion_page_pool.c | 3 --
>>>>>> drivers/staging/android/ion/ion_system_heap.c | 5 ----
>>>>>> 6 files changed, 65 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>>>>> b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644
>>>>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>>>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>>>>> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>>>>>> unsigned long arg)>
>>>>>> data.handle.handle = handle->id;
>>>>>>
>>>>>> break;
>>>>>>
>>>>>> }
>>>>>>
>>>>>> - case ION_IOC_SYNC:
>>>>>> - {
>>>>>> - ret = ion_sync_for_device(client, data.fd.fd);
>>>>>> - break;
>>>>>> - }
>>>>>
>>>>> You missed the case ION_IOC_SYNC: in compat_ion.c.
>>>>>
>>>>> While at it: Should we also remove the entire custom_ioctl infrastructure?
>>>>> It's entirely unused afaict, and for a pure buffer allocator I don't see
>>>>> any need to have custom ioctl.
>>>>
>>>> I second that, if you want to make ion a standard API, then we certainly don't
>>>> want any custom ioctl.
>>>>
>>>>> More code to remove potentially:
>>>>> - The entire compat ioctl stuff - would be an abi break, but I guess if we
>>>>> pick the 32bit abi and clean up the uapi headers we'll be mostly fine.
>>>>> would allow us to remove compat_ion.c entirely.
>>>>>
>>>>> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we
>>>>> still need to be able to import anything. All the cache flushing/mapping
>>>>> is done through dma-buf ops/ioctls.
>>>>>
>>>>>
>>>
>>> Good point to all of the above. I was considering keeping the import around
>>> for backwards compatibility reasons but given how much other stuff is being
>>> potentially broken, everything should just get ripped out.
>>
>> If you're ok with breaking the world, then I strongly suggest we go
>> through the uapi header and replace all types with the standard
>> fixed-width ones (__s32, __s64 and __u32, __u64). Allows us to remove all
>> the compat ioctl code :-)
>
> I think the other comments from your "botching-up ioctls" [1] also apply ;-)
> Namely - align structs to multiple of 64bit, add "flags" and properly
> verity user input returning -EINVAL.
>
> -Emil
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt
I'm more torn on this. There's a difference between dropping an old
ioctl/implicit caching vs. changing an actual ioctl ABI.
Maybe having obvious breakage is better than subtle though,
plus nobody has come begging me not to break the ABI yet.
I might leave this for right before we do the actual move
out of staging.
Thanks,
Laura
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-03 11:40 +0100 |
| Subject | Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging |
| Message-ID | <tgSMV-8mQ-11@gated-at.bofh.it> |
| In reply to | #1591485 |
On Thu, Mar 02, 2017 at 01:44:32PM -0800, Laura Abbott wrote: > Hi, > > There's been some recent discussions[1] about Ion-like frameworks. There's > apparently interest in just keeping Ion since it works reasonablly well. > This series does what should be the final clean ups for it to possibly be > moved out of staging. > > This includes the following: > - Some general clean up and removal of features that never got a lot of use > as far as I can tell. > - Fixing up the caching. This is the series I proposed back in December[2] > but never heard any feedback on. It will certainly break existing > applications that rely on the implicit caching. I'd rather make an effort > to move to a model that isn't going directly against the establishement > though. > - Fixing up the platform support. The devicetree approach was never well > recieved by DT maintainers. The proposal here is to think of Ion less as > specifying requirements and more of a framework for exposing memory to > userspace. > - CMA allocations now happen without the need of a dummy device structure. > This fixes a bunch of the reasons why I attempted to add devicetree > support before. > > I've had problems getting feedback in the past so if I don't hear any major > objections I'm going to send out with the RFC dropped to be picked up. > The only reason there isn't a patch to come out of staging is to discuss any > other changes to the ABI people might want. Once this comes out of staging, > I really don't want to mess with the ABI. > > Feedback appreciated. Imo looks all good. And I just realized that cross-checking with the TODO, the 2 items about _CUSTOM and _IMPORT ioctls I noted are already there. Otherwise I looked through the patches, looks all really reasonable. Wrt merging, my experience from destaging the android syncpt stuff was that merging the patches through the staging tree lead to lots of cross-tree issues with the gpu folks wanting to use that. Ion will probably run into similar things, so I'd propose we pull these cleanup patches and the eventual de-staging in throught drm. Yes that defacto means I'm also volunteering myself a bit :-) In the end we could put it all into drivers/gpu/ion or something like that. Thoughts? Greg? -Daniel > > Thanks, > Laura > > [1] https://marc.info/?l=linux-kernel&m=148699712602105&w=2 > [2] https://marc.info/?l=linaro-mm-sig&m=148176050802908&w=2 > > Laura Abbott (12): > staging: android: ion: Remove dmap_cnt > staging: android: ion: Remove alignment from allocation field > staging: android: ion: Duplicate sg_table > staging: android: ion: Call dma_map_sg for syncing and mapping > staging: android: ion: Remove page faulting support > staging: android: ion: Remove crufty cache support > staging: android: ion: Remove old platform support > cma: Store a name in the cma structure > cma: Introduce cma_for_each_area > staging: android: ion: Use CMA APIs directly > staging: android: ion: Make Ion heaps selectable > staging; android: ion: Enumerate all available heaps > > drivers/base/dma-contiguous.c | 5 +- > drivers/staging/android/ion/Kconfig | 51 ++-- > drivers/staging/android/ion/Makefile | 14 +- > drivers/staging/android/ion/hisilicon/Kconfig | 5 - > drivers/staging/android/ion/hisilicon/Makefile | 1 - > drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 --------- > drivers/staging/android/ion/ion-ioctl.c | 6 - > drivers/staging/android/ion/ion.c | 282 ++++++--------------- > drivers/staging/android/ion/ion.h | 5 +- > drivers/staging/android/ion/ion_carveout_heap.c | 16 +- > drivers/staging/android/ion/ion_chunk_heap.c | 15 +- > drivers/staging/android/ion/ion_cma_heap.c | 102 ++------ > drivers/staging/android/ion/ion_dummy_driver.c | 156 ------------ > drivers/staging/android/ion/ion_enumerate.c | 89 +++++++ > drivers/staging/android/ion/ion_of.c | 184 -------------- > drivers/staging/android/ion/ion_of.h | 37 --- > drivers/staging/android/ion/ion_page_pool.c | 3 - > drivers/staging/android/ion/ion_priv.h | 57 ++++- > drivers/staging/android/ion/ion_system_heap.c | 14 +- > drivers/staging/android/ion/tegra/Makefile | 1 - > drivers/staging/android/ion/tegra/tegra_ion.c | 80 ------ > include/linux/cma.h | 6 +- > mm/cma.c | 25 +- > mm/cma.h | 1 + > mm/cma_debug.c | 2 +- > 25 files changed, 312 insertions(+), 958 deletions(-) > delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig > delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile > delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c > delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c > create mode 100644 drivers/staging/android/ion/ion_enumerate.c > delete mode 100644 drivers/staging/android/ion/ion_of.c > delete mode 100644 drivers/staging/android/ion/ion_of.h > delete mode 100644 drivers/staging/android/ion/tegra/Makefile > delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c > > -- > 2.7.4 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-03 11:40 +0100 |
| Subject | Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging |
| Message-ID | <tgSMV-8mQ-9@gated-at.bofh.it> |
| In reply to | #1591844 |
On Fri, Mar 03, 2017 at 11:04:33AM +0100, Daniel Vetter wrote: > On Thu, Mar 02, 2017 at 01:44:32PM -0800, Laura Abbott wrote: > > Hi, > > > > There's been some recent discussions[1] about Ion-like frameworks. There's > > apparently interest in just keeping Ion since it works reasonablly well. > > This series does what should be the final clean ups for it to possibly be > > moved out of staging. > > > > This includes the following: > > - Some general clean up and removal of features that never got a lot of use > > as far as I can tell. > > - Fixing up the caching. This is the series I proposed back in December[2] > > but never heard any feedback on. It will certainly break existing > > applications that rely on the implicit caching. I'd rather make an effort > > to move to a model that isn't going directly against the establishement > > though. > > - Fixing up the platform support. The devicetree approach was never well > > recieved by DT maintainers. The proposal here is to think of Ion less as > > specifying requirements and more of a framework for exposing memory to > > userspace. > > - CMA allocations now happen without the need of a dummy device structure. > > This fixes a bunch of the reasons why I attempted to add devicetree > > support before. > > > > I've had problems getting feedback in the past so if I don't hear any major > > objections I'm going to send out with the RFC dropped to be picked up. > > The only reason there isn't a patch to come out of staging is to discuss any > > other changes to the ABI people might want. Once this comes out of staging, > > I really don't want to mess with the ABI. > > > > Feedback appreciated. > > Imo looks all good. And I just realized that cross-checking with the TODO, > the 2 items about _CUSTOM and _IMPORT ioctls I noted are already there. One more for the todo: Add rst/sphinx documentation for ION. That's also always a good excuse to review the internal interfaces and exported symbols. But we can do that after destaging ... -Daniel > > Otherwise I looked through the patches, looks all really reasonable. > > Wrt merging, my experience from destaging the android syncpt stuff was > that merging the patches through the staging tree lead to lots of > cross-tree issues with the gpu folks wanting to use that. Ion will > probably run into similar things, so I'd propose we pull these cleanup > patches and the eventual de-staging in throught drm. Yes that defacto > means I'm also volunteering myself a bit :-) > > In the end we could put it all into drivers/gpu/ion or something like > that. > > Thoughts? Greg? > -Daniel > > > > > > Thanks, > > Laura > > > > [1] https://marc.info/?l=linux-kernel&m=148699712602105&w=2 > > [2] https://marc.info/?l=linaro-mm-sig&m=148176050802908&w=2 > > > > Laura Abbott (12): > > staging: android: ion: Remove dmap_cnt > > staging: android: ion: Remove alignment from allocation field > > staging: android: ion: Duplicate sg_table > > staging: android: ion: Call dma_map_sg for syncing and mapping > > staging: android: ion: Remove page faulting support > > staging: android: ion: Remove crufty cache support > > staging: android: ion: Remove old platform support > > cma: Store a name in the cma structure > > cma: Introduce cma_for_each_area > > staging: android: ion: Use CMA APIs directly > > staging: android: ion: Make Ion heaps selectable > > staging; android: ion: Enumerate all available heaps > > > > drivers/base/dma-contiguous.c | 5 +- > > drivers/staging/android/ion/Kconfig | 51 ++-- > > drivers/staging/android/ion/Makefile | 14 +- > > drivers/staging/android/ion/hisilicon/Kconfig | 5 - > > drivers/staging/android/ion/hisilicon/Makefile | 1 - > > drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 --------- > > drivers/staging/android/ion/ion-ioctl.c | 6 - > > drivers/staging/android/ion/ion.c | 282 ++++++--------------- > > drivers/staging/android/ion/ion.h | 5 +- > > drivers/staging/android/ion/ion_carveout_heap.c | 16 +- > > drivers/staging/android/ion/ion_chunk_heap.c | 15 +- > > drivers/staging/android/ion/ion_cma_heap.c | 102 ++------ > > drivers/staging/android/ion/ion_dummy_driver.c | 156 ------------ > > drivers/staging/android/ion/ion_enumerate.c | 89 +++++++ > > drivers/staging/android/ion/ion_of.c | 184 -------------- > > drivers/staging/android/ion/ion_of.h | 37 --- > > drivers/staging/android/ion/ion_page_pool.c | 3 - > > drivers/staging/android/ion/ion_priv.h | 57 ++++- > > drivers/staging/android/ion/ion_system_heap.c | 14 +- > > drivers/staging/android/ion/tegra/Makefile | 1 - > > drivers/staging/android/ion/tegra/tegra_ion.c | 80 ------ > > include/linux/cma.h | 6 +- > > mm/cma.c | 25 +- > > mm/cma.h | 1 + > > mm/cma_debug.c | 2 +- > > 25 files changed, 312 insertions(+), 958 deletions(-) > > delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig > > delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile > > delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c > > delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c > > create mode 100644 drivers/staging/android/ion/ion_enumerate.c > > delete mode 100644 drivers/staging/android/ion/ion_of.c > > delete mode 100644 drivers/staging/android/ion/ion_of.h > > delete mode 100644 drivers/staging/android/ion/tegra/Makefile > > delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c > > > > -- > > 2.7.4 > > > > -- > > To unsubscribe, send a message with 'unsubscribe linux-mm' in > > the body to majordomo@kvack.org. For more info on Linux MM, > > see: http://www.linux-mm.org/ . > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Gaignard <benjamin.gaignard@linaro.org> |
|---|---|
| Date | 2017-03-03 15:50 +0100 |
| Message-ID | <tgWGT-2zu-37@gated-at.bofh.it> |
| In reply to | #1591847 |
2017-03-03 11:27 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Mar 03, 2017 at 11:04:33AM +0100, Daniel Vetter wrote: >> On Thu, Mar 02, 2017 at 01:44:32PM -0800, Laura Abbott wrote: >> > Hi, >> > >> > There's been some recent discussions[1] about Ion-like frameworks. There's >> > apparently interest in just keeping Ion since it works reasonablly well. >> > This series does what should be the final clean ups for it to possibly be >> > moved out of staging. >> > >> > This includes the following: >> > - Some general clean up and removal of features that never got a lot of use >> > as far as I can tell. >> > - Fixing up the caching. This is the series I proposed back in December[2] >> > but never heard any feedback on. It will certainly break existing >> > applications that rely on the implicit caching. I'd rather make an effort >> > to move to a model that isn't going directly against the establishement >> > though. >> > - Fixing up the platform support. The devicetree approach was never well >> > recieved by DT maintainers. The proposal here is to think of Ion less as >> > specifying requirements and more of a framework for exposing memory to >> > userspace. >> > - CMA allocations now happen without the need of a dummy device structure. >> > This fixes a bunch of the reasons why I attempted to add devicetree >> > support before. >> > >> > I've had problems getting feedback in the past so if I don't hear any major >> > objections I'm going to send out with the RFC dropped to be picked up. >> > The only reason there isn't a patch to come out of staging is to discuss any >> > other changes to the ABI people might want. Once this comes out of staging, >> > I really don't want to mess with the ABI. >> > >> > Feedback appreciated. >> >> Imo looks all good. And I just realized that cross-checking with the TODO, >> the 2 items about _CUSTOM and _IMPORT ioctls I noted are already there. > > One more for the todo: Add rst/sphinx documentation for ION. That's also > always a good excuse to review the internal interfaces and exported > symbols. But we can do that after destaging ... > -Daniel Removing alignment looks good for me but why not also remove it from struct ion_allocation_data since the field become useless ? Also does someone use ion_user_handle_t handle ? Can we directly export a dma-buf file descriptor ? Benjamin > >> >> Otherwise I looked through the patches, looks all really reasonable. >> >> Wrt merging, my experience from destaging the android syncpt stuff was >> that merging the patches through the staging tree lead to lots of >> cross-tree issues with the gpu folks wanting to use that. Ion will >> probably run into similar things, so I'd propose we pull these cleanup >> patches and the eventual de-staging in throught drm. Yes that defacto >> means I'm also volunteering myself a bit :-) >> >> In the end we could put it all into drivers/gpu/ion or something like >> that. >> >> Thoughts? Greg? >> -Daniel >> >> >> > >> > Thanks, >> > Laura >> > >> > [1] https://marc.info/?l=linux-kernel&m=148699712602105&w=2 >> > [2] https://marc.info/?l=linaro-mm-sig&m=148176050802908&w=2 >> > >> > Laura Abbott (12): >> > staging: android: ion: Remove dmap_cnt >> > staging: android: ion: Remove alignment from allocation field >> > staging: android: ion: Duplicate sg_table >> > staging: android: ion: Call dma_map_sg for syncing and mapping >> > staging: android: ion: Remove page faulting support >> > staging: android: ion: Remove crufty cache support >> > staging: android: ion: Remove old platform support >> > cma: Store a name in the cma structure >> > cma: Introduce cma_for_each_area >> > staging: android: ion: Use CMA APIs directly >> > staging: android: ion: Make Ion heaps selectable >> > staging; android: ion: Enumerate all available heaps >> > >> > drivers/base/dma-contiguous.c | 5 +- >> > drivers/staging/android/ion/Kconfig | 51 ++-- >> > drivers/staging/android/ion/Makefile | 14 +- >> > drivers/staging/android/ion/hisilicon/Kconfig | 5 - >> > drivers/staging/android/ion/hisilicon/Makefile | 1 - >> > drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 --------- >> > drivers/staging/android/ion/ion-ioctl.c | 6 - >> > drivers/staging/android/ion/ion.c | 282 ++++++--------------- >> > drivers/staging/android/ion/ion.h | 5 +- >> > drivers/staging/android/ion/ion_carveout_heap.c | 16 +- >> > drivers/staging/android/ion/ion_chunk_heap.c | 15 +- >> > drivers/staging/android/ion/ion_cma_heap.c | 102 ++------ >> > drivers/staging/android/ion/ion_dummy_driver.c | 156 ------------ >> > drivers/staging/android/ion/ion_enumerate.c | 89 +++++++ >> > drivers/staging/android/ion/ion_of.c | 184 -------------- >> > drivers/staging/android/ion/ion_of.h | 37 --- >> > drivers/staging/android/ion/ion_page_pool.c | 3 - >> > drivers/staging/android/ion/ion_priv.h | 57 ++++- >> > drivers/staging/android/ion/ion_system_heap.c | 14 +- >> > drivers/staging/android/ion/tegra/Makefile | 1 - >> > drivers/staging/android/ion/tegra/tegra_ion.c | 80 ------ >> > include/linux/cma.h | 6 +- >> > mm/cma.c | 25 +- >> > mm/cma.h | 1 + >> > mm/cma_debug.c | 2 +- >> > 25 files changed, 312 insertions(+), 958 deletions(-) >> > delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig >> > delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile >> > delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c >> > delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c >> > create mode 100644 drivers/staging/android/ion/ion_enumerate.c >> > delete mode 100644 drivers/staging/android/ion/ion_of.c >> > delete mode 100644 drivers/staging/android/ion/ion_of.h >> > delete mode 100644 drivers/staging/android/ion/tegra/Makefile >> > delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c >> > >> > -- >> > 2.7.4 >> > >> > -- >> > To unsubscribe, send a message with 'unsubscribe linux-mm' in >> > the body to majordomo@kvack.org. For more info on Linux MM, >> > see: http://www.linux-mm.org/ . >> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Laurent Pinchart <laurent.pinchart@ideasonboard.com> |
|---|---|
| Date | 2017-03-03 17:50 +0100 |
| Message-ID | <tgYyZ-3Rj-5@gated-at.bofh.it> |
| In reply to | #1591844 |
Hi Daniel, On Friday 03 Mar 2017 11:04:33 Daniel Vetter wrote: > On Thu, Mar 02, 2017 at 01:44:32PM -0800, Laura Abbott wrote: > > Hi, > > > > There's been some recent discussions[1] about Ion-like frameworks. There's > > apparently interest in just keeping Ion since it works reasonablly well. > > This series does what should be the final clean ups for it to possibly be > > moved out of staging. > > > > This includes the following: > > - Some general clean up and removal of features that never got a lot of > > use as far as I can tell. > > > > - Fixing up the caching. This is the series I proposed back in December[2] > > but never heard any feedback on. It will certainly break existing > > applications that rely on the implicit caching. I'd rather make an > > effort to move to a model that isn't going directly against the > > establishement though. > > > > - Fixing up the platform support. The devicetree approach was never well > > recieved by DT maintainers. The proposal here is to think of Ion less as > > specifying requirements and more of a framework for exposing memory to > > userspace. > > > > - CMA allocations now happen without the need of a dummy device structure. > > This fixes a bunch of the reasons why I attempted to add devicetree > > support before. > > > > I've had problems getting feedback in the past so if I don't hear any > > major objections I'm going to send out with the RFC dropped to be picked > > up. The only reason there isn't a patch to come out of staging is to > > discuss any other changes to the ABI people might want. Once this comes > > out of staging, I really don't want to mess with the ABI. > > > > Feedback appreciated. > > Imo looks all good. And I just realized that cross-checking with the TODO, > the 2 items about _CUSTOM and _IMPORT ioctls I noted are already there. > > Otherwise I looked through the patches, looks all really reasonable. Two more items that need to be addressed in my opinion : - Let's not export the ion_client API, we don't want drivers to be ion- specific. Only the dma-buf interface should be visible to drivers. - I haven't seen any proposal how a heap-based solution could be used in a generic distribution. This needs to be figured out before committing to any API/ABI. > Wrt merging, my experience from destaging the android syncpt stuff was > that merging the patches through the staging tree lead to lots of > cross-tree issues with the gpu folks wanting to use that. Ion will > probably run into similar things, so I'd propose we pull these cleanup > patches and the eventual de-staging in throught drm. Yes that defacto > means I'm also volunteering myself a bit :-) > > In the end we could put it all into drivers/gpu/ion or something like > that. > > Thoughts? Greg? > -Daniel > > > Thanks, > > Laura > > > > [1] https://marc.info/?l=linux-kernel&m=148699712602105&w=2 > > [2] https://marc.info/?l=linaro-mm-sig&m=148176050802908&w=2 > > > > Laura Abbott (12): > > staging: android: ion: Remove dmap_cnt > > staging: android: ion: Remove alignment from allocation field > > staging: android: ion: Duplicate sg_table > > staging: android: ion: Call dma_map_sg for syncing and mapping > > staging: android: ion: Remove page faulting support > > staging: android: ion: Remove crufty cache support > > staging: android: ion: Remove old platform support > > cma: Store a name in the cma structure > > cma: Introduce cma_for_each_area > > staging: android: ion: Use CMA APIs directly > > staging: android: ion: Make Ion heaps selectable > > staging; android: ion: Enumerate all available heaps > > > > drivers/base/dma-contiguous.c | 5 +- > > drivers/staging/android/ion/Kconfig | 51 ++-- > > drivers/staging/android/ion/Makefile | 14 +- > > drivers/staging/android/ion/hisilicon/Kconfig | 5 - > > drivers/staging/android/ion/hisilicon/Makefile | 1 - > > drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 --------- > > drivers/staging/android/ion/ion-ioctl.c | 6 - > > drivers/staging/android/ion/ion.c | 282 > > ++++++--------------- drivers/staging/android/ion/ion.h > > | 5 +- > > drivers/staging/android/ion/ion_carveout_heap.c | 16 +- > > drivers/staging/android/ion/ion_chunk_heap.c | 15 +- > > drivers/staging/android/ion/ion_cma_heap.c | 102 ++------ > > drivers/staging/android/ion/ion_dummy_driver.c | 156 ------------ > > drivers/staging/android/ion/ion_enumerate.c | 89 +++++++ > > drivers/staging/android/ion/ion_of.c | 184 -------------- > > drivers/staging/android/ion/ion_of.h | 37 --- > > drivers/staging/android/ion/ion_page_pool.c | 3 - > > drivers/staging/android/ion/ion_priv.h | 57 ++++- > > drivers/staging/android/ion/ion_system_heap.c | 14 +- > > drivers/staging/android/ion/tegra/Makefile | 1 - > > drivers/staging/android/ion/tegra/tegra_ion.c | 80 ------ > > include/linux/cma.h | 6 +- > > mm/cma.c | 25 +- > > mm/cma.h | 1 + > > mm/cma_debug.c | 2 +- > > 25 files changed, 312 insertions(+), 958 deletions(-) > > delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig > > delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile > > delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c > > delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c > > create mode 100644 drivers/staging/android/ion/ion_enumerate.c > > delete mode 100644 drivers/staging/android/ion/ion_of.c > > delete mode 100644 drivers/staging/android/ion/ion_of.h > > delete mode 100644 drivers/staging/android/ion/tegra/Makefile > > delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c -- Regards, Laurent Pinchart
[toc] | [prev] | [next] | [standalone]
| From | Laura Abbott <labbott@redhat.com> |
|---|---|
| Date | 2017-03-03 20:20 +0100 |
| Subject | Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging |
| Message-ID | <th0Ua-5DA-15@gated-at.bofh.it> |
| In reply to | #1592127 |
On 03/03/2017 08:45 AM, Laurent Pinchart wrote: > Hi Daniel, > > On Friday 03 Mar 2017 11:04:33 Daniel Vetter wrote: >> On Thu, Mar 02, 2017 at 01:44:32PM -0800, Laura Abbott wrote: >>> Hi, >>> >>> There's been some recent discussions[1] about Ion-like frameworks. There's >>> apparently interest in just keeping Ion since it works reasonablly well. >>> This series does what should be the final clean ups for it to possibly be >>> moved out of staging. >>> >>> This includes the following: >>> - Some general clean up and removal of features that never got a lot of >>> use as far as I can tell. >>> >>> - Fixing up the caching. This is the series I proposed back in December[2] >>> but never heard any feedback on. It will certainly break existing >>> applications that rely on the implicit caching. I'd rather make an >>> effort to move to a model that isn't going directly against the >>> establishement though. >>> >>> - Fixing up the platform support. The devicetree approach was never well >>> recieved by DT maintainers. The proposal here is to think of Ion less as >>> specifying requirements and more of a framework for exposing memory to >>> userspace. >>> >>> - CMA allocations now happen without the need of a dummy device structure. >>> This fixes a bunch of the reasons why I attempted to add devicetree >>> support before. >>> >>> I've had problems getting feedback in the past so if I don't hear any >>> major objections I'm going to send out with the RFC dropped to be picked >>> up. The only reason there isn't a patch to come out of staging is to >>> discuss any other changes to the ABI people might want. Once this comes >>> out of staging, I really don't want to mess with the ABI. >>> >>> Feedback appreciated. >> >> Imo looks all good. And I just realized that cross-checking with the TODO, >> the 2 items about _CUSTOM and _IMPORT ioctls I noted are already there. >> >> Otherwise I looked through the patches, looks all really reasonable. > > Two more items that need to be addressed in my opinion : > > - Let's not export the ion_client API, we don't want drivers to be ion- > specific. Only the dma-buf interface should be visible to drivers. > Yes, that's a good point. I never heard back from anyone about a need for in kernel allocation via Ion. Thanks, Laura
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-06 11:50 +0100 |
| Subject | Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging |
| Message-ID | <thYnf-6U9-15@gated-at.bofh.it> |
| In reply to | #1592127 |
On Fri, Mar 03, 2017 at 06:45:40PM +0200, Laurent Pinchart wrote:
> - I haven't seen any proposal how a heap-based solution could be used in a
> generic distribution. This needs to be figured out before committing to any
> API/ABI.
Two replies from my side:
- Just because a patch doesn't solve world hunger isn't really a good
reason to reject it.
- Heap doesn't mean its not resizeable (but I'm not sure that's really
your concern).
- Imo ION is very much part of the picture here to solve this for real. We
need to bits:
* Be able to allocate memory from specific pools, not going through a
specific driver. ION gives us that interface. This is e.g. also needed
for "special" memory, like SMA tries to expose.
* Some way to figure out how&where to allocate the buffer object. This
is purely a userspace problem, and this is the part the unix memory
allocator tries to solve. There's no plans in there for big kernel
changes, instead userspace does a dance to reconcile all the
constraints, and one of the constraints might be "you have to allocate
this from this special ION heap". The only thing the kernel needs to
expose is which devices use which ION heaps (we kinda do that
already), and maybe some hints of how they can be generalized (but I
guess stuff like "minimal pagesize of x KB" is also fulfilled by any
CMA heap is knowledge userspace needs).
Again I think waiting for this to be fully implemented before we merge any
part is going to just kill any upstreaming efforts. ION in itself, without
the full buffer negotiation dance seems clearly useful (also for stuff
like SMA), and having it merged will help with moving the buffer
allocation dance forward.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Laurent Pinchart <laurent.pinchart@ideasonboard.com> |
|---|---|
| Date | 2017-03-06 16:10 +0100 |
| Message-ID | <ti2qS-1v4-25@gated-at.bofh.it> |
| In reply to | #1593180 |
Hi Daniel, On Monday 06 Mar 2017 11:38:20 Daniel Vetter wrote: > On Fri, Mar 03, 2017 at 06:45:40PM +0200, Laurent Pinchart wrote: > > - I haven't seen any proposal how a heap-based solution could be used in a > > generic distribution. This needs to be figured out before committing to > > any API/ABI. > > Two replies from my side: > > - Just because a patch doesn't solve world hunger isn't really a good > reason to reject it. As long as it goes in the right direction, sure :-) The points I mentioned were to be interpreted that way, I want to make sure we're not going in a dead-end (or worse, driving full speed into a wall). > - Heap doesn't mean its not resizeable (but I'm not sure that's really > your concern). Not really, no. Heap is another word to mean pool here. It might not be the best term in this context as it has a precise meaning in the context of memory allocation, but that's a detail. > - Imo ION is very much part of the picture here to solve this for real. We > need to bits: > > * Be able to allocate memory from specific pools, not going through a > specific driver. ION gives us that interface. This is e.g. also needed > for "special" memory, like SMA tries to expose. > > * Some way to figure out how&where to allocate the buffer object. This > is purely a userspace problem, and this is the part the unix memory > allocator tries to solve. There's no plans in there for big kernel > changes, instead userspace does a dance to reconcile all the > constraints, and one of the constraints might be "you have to allocate > this from this special ION heap". The only thing the kernel needs to > expose is which devices use which ION heaps (we kinda do that > already), and maybe some hints of how they can be generalized (but I > guess stuff like "minimal pagesize of x KB" is also fulfilled by any > CMA heap is knowledge userspace needs). The constraint solver could live in userspace, I'm open to a solution that would go in that direction, but it will require help from the kernel to fetch the constraints from the devices that need to be involved in buffer sharing. Given a userspace constraint resolver, the interface with the kernel allocator will likely be based on pools. I'm not opposed to that, as long as pool are identified by opaque handles. I don't want userspace to know about the meaning of any particular ION heap. Application must not attempt to "allocate from CMA" for instance, that would lock us to a crazy API that will grow completely out of hands as vendors will start adding all kind of custom heaps, and applications will have to follow (or will be patched out-of-tree by vendors). > Again I think waiting for this to be fully implemented before we merge any > part is going to just kill any upstreaming efforts. ION in itself, without > the full buffer negotiation dance seems clearly useful (also for stuff > like SMA), and having it merged will help with moving the buffer > allocation dance forward. Again I'm not opposed to a kernel allocator based on pools/heaps, as long as - pools/heaps stay internal to the kernel and are not directly exposed to userspace - a reasonable way to size the different kinds of pools in a generic distribution kernel can be found -- Regards, Laurent Pinchart
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-03-06 17:40 +0100 |
| Subject | Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging |
| Message-ID | <ti3PY-2mY-23@gated-at.bofh.it> |
| In reply to | #1593467 |
On Mon, Mar 06, 2017 at 05:02:05PM +0200, Laurent Pinchart wrote: > Hi Daniel, > > On Monday 06 Mar 2017 11:38:20 Daniel Vetter wrote: > > On Fri, Mar 03, 2017 at 06:45:40PM +0200, Laurent Pinchart wrote: > > > - I haven't seen any proposal how a heap-based solution could be used in a > > > generic distribution. This needs to be figured out before committing to > > > any API/ABI. > > > > Two replies from my side: > > > > - Just because a patch doesn't solve world hunger isn't really a good > > reason to reject it. > > As long as it goes in the right direction, sure :-) The points I mentioned > were to be interpreted that way, I want to make sure we're not going in a > dead-end (or worse, driving full speed into a wall). > > > - Heap doesn't mean its not resizeable (but I'm not sure that's really > > your concern). > > Not really, no. Heap is another word to mean pool here. It might not be the > best term in this context as it has a precise meaning in the context of memory > allocation, but that's a detail. > > > - Imo ION is very much part of the picture here to solve this for real. We > > need to bits: > > > > * Be able to allocate memory from specific pools, not going through a > > specific driver. ION gives us that interface. This is e.g. also needed > > for "special" memory, like SMA tries to expose. > > > > * Some way to figure out how&where to allocate the buffer object. This > > is purely a userspace problem, and this is the part the unix memory > > allocator tries to solve. There's no plans in there for big kernel > > changes, instead userspace does a dance to reconcile all the > > constraints, and one of the constraints might be "you have to allocate > > this from this special ION heap". The only thing the kernel needs to > > expose is which devices use which ION heaps (we kinda do that > > already), and maybe some hints of how they can be generalized (but I > > guess stuff like "minimal pagesize of x KB" is also fulfilled by any > > CMA heap is knowledge userspace needs). > > The constraint solver could live in userspace, I'm open to a solution that > would go in that direction, but it will require help from the kernel to fetch > the constraints from the devices that need to be involved in buffer sharing. > > Given a userspace constraint resolver, the interface with the kernel allocator > will likely be based on pools. I'm not opposed to that, as long as pool are > identified by opaque handles. I don't want userspace to know about the meaning > of any particular ION heap. Application must not attempt to "allocate from > CMA" for instance, that would lock us to a crazy API that will grow completely > out of hands as vendors will start adding all kind of custom heaps, and > applications will have to follow (or will be patched out-of-tree by vendors). > > > Again I think waiting for this to be fully implemented before we merge any > > part is going to just kill any upstreaming efforts. ION in itself, without > > the full buffer negotiation dance seems clearly useful (also for stuff > > like SMA), and having it merged will help with moving the buffer > > allocation dance forward. > > Again I'm not opposed to a kernel allocator based on pools/heaps, as long as > > - pools/heaps stay internal to the kernel and are not directly exposed to > userspace Agreed (and I think ION doesn't have fixed pools afaik, just kinda conventions, at least after Laura's patches). But on a fixed board with a fixed DT (for the cma regions) and fixed .config (for the generic heaps) you can hardcode your heaps. You'll make your code non-portable, but hey that's not our problem imo. E.g. board-specific code can also hard-code how to wire connectors and which one is which in kms (and I've seen this). I don't think the possibility of abusing the uabi should be a good reason to prevent it from merging. Anything that provides something with indirect connections can be abused by hardcoding the names or the indizes. We do have a TODO entry that talks about exposing the device -> cma heap link in sysfs or somewhere. I'm not versed enough to know whether Laura's patches fixed that, this here mostly seems to tackle the fundamentals of the dma api abuse first. > - a reasonable way to size the different kinds of pools in a generic > distribution kernel can be found So for the CMA heaps, you can't resize them at runtime, for obvious reasons. For boot-time you can adjust them through DT, and I thought everyone agreed that for different use-cases you might need to adjust your reserved regions. For all other heaps, they just use the normal allocator functions (e.g. alloc_pages). There's not limit on those except OOM, so nothing to adjust really. I guess I'm still not entirely clear on your "memory pool" concern ... If it's just the word, we have lots of auto-resizing heaps/pools all around. And if it's just sizing, I think that's already solved as good as possible (assuming there's not a silly limit on the system heap that we should remove ...). Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
Page 2 of 4 — ← Prev page 1 [2] 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web