| Message ID | 1545639585-24785-1-git-send-email-saberlily.xia@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: android: ion: move map_kernel to ion_dma_buf_kmap | expand |
On 12/24/18 12:19 AM, Qing Xia wrote: > Now, as Google's user guide, if userspace need clean ION buffer's > cache, they should call ioctl(fd, DMA_BUF_IOCTL_SYNC, sync). Then > we found that ion_dma_buf_begin_cpu_access/ion_dma_buf_end_cpu_access > will do ION buffer's map_kernel, it's not necessary. And if usersapce > only need clean cache, they will call ion_dma_buf_end_cpu_access by > dmabuf's ioctl, ION buffer's kmap_cnt will be wrong value -1, then > driver could not get right kernel vaddr by dma_buf_kmap. > The problem is this subtly violates how dma_buf is supposed to work. All setup is supposed to happen in begin_cpu_access. I agree calling map kernel each time isn't great but I think this needs to be worked out with dma_buf. Thanks, Laura > Signed-off-by: Qing Xia <saberlily.xia@hisilicon.com> > --- > drivers/staging/android/ion/ion.c | 46 ++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 25 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index 9907332..f7e2812 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -303,45 +303,47 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf) > static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset) > { > struct ion_buffer *buffer = dmabuf->priv; > + void *vaddr; > > - return buffer->vaddr + offset * PAGE_SIZE; > + if (buffer->heap->ops->map_kernel) { > + mutex_lock(&buffer->lock); > + vaddr = ion_buffer_kmap_get(buffer); > + mutex_unlock(&buffer->lock); > + if (IS_ERR(vaddr)) > + return vaddr; > + > + return vaddr + offset * PAGE_SIZE; > + } > + > + return NULL; > } > > static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset, > void *ptr) > { > + struct ion_buffer *buffer = dmabuf->priv; > + > + if (buffer->heap->ops->map_kernel) { > + mutex_lock(&buffer->lock); > + ion_buffer_kmap_put(buffer); > + mutex_unlock(&buffer->lock); > + } > } > > static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > enum dma_data_direction direction) > { > struct ion_buffer *buffer = dmabuf->priv; > - void *vaddr; > struct ion_dma_buf_attachment *a; > - int ret = 0; > - > - /* > - * 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); > - if (IS_ERR(vaddr)) { > - ret = PTR_ERR(vaddr); > - goto unlock; > - } > - mutex_unlock(&buffer->lock); > - } > > mutex_lock(&buffer->lock); > list_for_each_entry(a, &buffer->attachments, list) { > dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, > direction); > } > - > -unlock: > mutex_unlock(&buffer->lock); > - return ret; > + > + return 0; > } > > static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > @@ -350,12 +352,6 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > struct ion_buffer *buffer = dmabuf->priv; > struct ion_dma_buf_attachment *a; > > - if (buffer->heap->ops->map_kernel) { > - mutex_lock(&buffer->lock); > - ion_buffer_kmap_put(buffer); > - mutex_unlock(&buffer->lock); > - } > - > mutex_lock(&buffer->lock); > list_for_each_entry(a, &buffer->attachments, list) { > dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents, >
On 2019/1/3 6:36, Laura Abbott wrote: > On 12/24/18 12:19 AM, Qing Xia wrote: >> Now, as Google's user guide, if userspace need clean ION buffer's >> cache, they should call ioctl(fd, DMA_BUF_IOCTL_SYNC, sync). Then >> we found that ion_dma_buf_begin_cpu_access/ion_dma_buf_end_cpu_access >> will do ION buffer's map_kernel, it's not necessary. And if usersapce >> only need clean cache, they will call ion_dma_buf_end_cpu_access by >> dmabuf's ioctl, ION buffer's kmap_cnt will be wrong value -1, then >> driver could not get right kernel vaddr by dma_buf_kmap. >> > > The problem is this subtly violates how dma_buf is supposed > to work. All setup is supposed to happen in begin_cpu_access. > I agree calling map kernel each time isn't great but I think > this needs to be worked out with dma_buf. > > Thanks, > Laura > Thanks for your explanation. >> Signed-off-by: Qing Xia <saberlily.xia@hisilicon.com> >> --- >> drivers/staging/android/ion/ion.c | 46 ++++++++++++++++++--------------------- >> 1 file changed, 21 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c >> index 9907332..f7e2812 100644 >> --- a/drivers/staging/android/ion/ion.c >> +++ b/drivers/staging/android/ion/ion.c >> @@ -303,45 +303,47 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf) >> static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset) >> { >> struct ion_buffer *buffer = dmabuf->priv; >> + void *vaddr; >> - return buffer->vaddr + offset * PAGE_SIZE; >> + if (buffer->heap->ops->map_kernel) { >> + mutex_lock(&buffer->lock); >> + vaddr = ion_buffer_kmap_get(buffer); >> + mutex_unlock(&buffer->lock); >> + if (IS_ERR(vaddr)) >> + return vaddr; >> + >> + return vaddr + offset * PAGE_SIZE; >> + } >> + >> + return NULL; >> } >> static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset, >> void *ptr) >> { >> + struct ion_buffer *buffer = dmabuf->priv; >> + >> + if (buffer->heap->ops->map_kernel) { >> + mutex_lock(&buffer->lock); >> + ion_buffer_kmap_put(buffer); >> + mutex_unlock(&buffer->lock); >> + } >> } >> static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, >> enum dma_data_direction direction) >> { >> struct ion_buffer *buffer = dmabuf->priv; >> - void *vaddr; >> struct ion_dma_buf_attachment *a; >> - int ret = 0; >> - >> - /* >> - * 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); >> - if (IS_ERR(vaddr)) { >> - ret = PTR_ERR(vaddr); >> - goto unlock; >> - } >> - mutex_unlock(&buffer->lock); >> - } >> mutex_lock(&buffer->lock); >> list_for_each_entry(a, &buffer->attachments, list) { >> dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, >> direction); >> } >> - >> -unlock: >> mutex_unlock(&buffer->lock); >> - return ret; >> + >> + return 0; >> } >> static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, >> @@ -350,12 +352,6 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, >> struct ion_buffer *buffer = dmabuf->priv; >> struct ion_dma_buf_attachment *a; >> - if (buffer->heap->ops->map_kernel) { >> - mutex_lock(&buffer->lock); >> - ion_buffer_kmap_put(buffer); >> - mutex_unlock(&buffer->lock); >> - } >> - >> mutex_lock(&buffer->lock); >> list_for_each_entry(a, &buffer->attachments, list) { >> dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents, >> > > > . >
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 9907332..f7e2812 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -303,45 +303,47 @@ static void ion_dma_buf_release(struct dma_buf *dmabuf) static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long offset) { struct ion_buffer *buffer = dmabuf->priv; + void *vaddr; - return buffer->vaddr + offset * PAGE_SIZE; + if (buffer->heap->ops->map_kernel) { + mutex_lock(&buffer->lock); + vaddr = ion_buffer_kmap_get(buffer); + mutex_unlock(&buffer->lock); + if (IS_ERR(vaddr)) + return vaddr; + + return vaddr + offset * PAGE_SIZE; + } + + return NULL; } static void ion_dma_buf_kunmap(struct dma_buf *dmabuf, unsigned long offset, void *ptr) { + struct ion_buffer *buffer = dmabuf->priv; + + if (buffer->heap->ops->map_kernel) { + mutex_lock(&buffer->lock); + ion_buffer_kmap_put(buffer); + mutex_unlock(&buffer->lock); + } } static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction) { struct ion_buffer *buffer = dmabuf->priv; - void *vaddr; struct ion_dma_buf_attachment *a; - int ret = 0; - - /* - * 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); - if (IS_ERR(vaddr)) { - ret = PTR_ERR(vaddr); - goto unlock; - } - mutex_unlock(&buffer->lock); - } mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, direction); } - -unlock: mutex_unlock(&buffer->lock); - return ret; + + return 0; } static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, @@ -350,12 +352,6 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, struct ion_buffer *buffer = dmabuf->priv; struct ion_dma_buf_attachment *a; - if (buffer->heap->ops->map_kernel) { - mutex_lock(&buffer->lock); - ion_buffer_kmap_put(buffer); - mutex_unlock(&buffer->lock); - } - mutex_lock(&buffer->lock); list_for_each_entry(a, &buffer->attachments, list) { dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
Now, as Google's user guide, if userspace need clean ION buffer's cache, they should call ioctl(fd, DMA_BUF_IOCTL_SYNC, sync). Then we found that ion_dma_buf_begin_cpu_access/ion_dma_buf_end_cpu_access will do ION buffer's map_kernel, it's not necessary. And if usersapce only need clean cache, they will call ion_dma_buf_end_cpu_access by dmabuf's ioctl, ION buffer's kmap_cnt will be wrong value -1, then driver could not get right kernel vaddr by dma_buf_kmap. Signed-off-by: Qing Xia <saberlily.xia@hisilicon.com> --- drivers/staging/android/ion/ion.c | 46 ++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 25 deletions(-)