* [PATCH 0/3] Small i915/exynos prime cleanup
@ 2013-08-07 9:15 Daniel Vetter
2013-08-07 9:15 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Daniel Vetter @ 2013-08-07 9:15 UTC (permalink / raw)
To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development
Hi all,
I need to get my prime fixes in since they're blocking QA from running -nightly
prime tests. Which is a prerequisite of mine before I start touching dma-buf for
real to look at fencing and ww-mutex integration for i915.
These three patches are just a bit of prep cleanup and one bugfix that Maarten
spotted.
Cheers, Daniel
Daniel Vetter (3):
drm: use common drm_gem_dmabuf_release in i915/exynos drivers
drm/i915: unpin backing storage in dmabuf_unmap
drm/i915: explicit store base gem object in dma_buf->priv
drivers/gpu/drm/drm_prime.c | 3 ++-
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +-------------------
drivers/gpu/drm/i915/i915_gem_dmabuf.c | 34 +++++++++++++-----------------
include/drm/drmP.h | 1 +
4 files changed, 19 insertions(+), 42 deletions(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 9:15 [PATCH 0/3] Small i915/exynos prime cleanup Daniel Vetter @ 2013-08-07 9:15 ` Daniel Vetter 2013-08-07 9:37 ` [Intel-gfx] " Chris Wilson 2013-08-07 9:40 ` Inki Dae 2013-08-07 9:15 ` [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap Daniel Vetter 2013-08-07 9:15 ` [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv Daniel Vetter 2 siblings, 2 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 9:15 UTC (permalink / raw) To: DRI Development; +Cc: Inki Dae, Daniel Vetter, Intel Graphics Development Note that this is slightly tricky since both drivers store their native objects in dma_buf->priv. But both also embed the base drm_gem_object at the first position, so the implicit cast is ok. To use the release helper we need to export it, too. Cc: Inki Dae <inki.dae@samsung.com> Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_prime.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +---------------------- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 13 +------------ include/drm/drmP.h | 1 + 4 files changed, 5 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 85e450e..a35f206 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -192,7 +192,7 @@ static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, /* nothing to be done here */ } -static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +void drm_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv; @@ -202,6 +202,7 @@ static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) drm_gem_object_unreference_unlocked(obj); } } +EXPORT_SYMBOL(drm_gem_dmabuf_release); static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index a0f997e..3cd56e1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -127,27 +127,6 @@ static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach, /* Nothing to do. */ } -static void exynos_dmabuf_release(struct dma_buf *dmabuf) -{ - struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv; - - /* - * exynos_dmabuf_release() call means that file object's - * f_count is 0 and it calls drm_gem_object_handle_unreference() - * to drop the references that these values had been increased - * at drm_prime_handle_to_fd() - */ - if (exynos_gem_obj->base.export_dma_buf == dmabuf) { - exynos_gem_obj->base.export_dma_buf = NULL; - - /* - * drop this gem object refcount to release allocated buffer - * and resources. - */ - drm_gem_object_unreference_unlocked(&exynos_gem_obj->base); - } -} - static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) { @@ -193,7 +172,7 @@ static struct dma_buf_ops exynos_dmabuf_ops = { .kunmap = exynos_gem_dmabuf_kunmap, .kunmap_atomic = exynos_gem_dmabuf_kunmap_atomic, .mmap = exynos_gem_dmabuf_mmap, - .release = exynos_dmabuf_release, + .release = drm_gem_dmabuf_release, }; struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev, diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index f2e185c..63ee1a9 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -90,17 +90,6 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, kfree(sg); } -static void i915_gem_dmabuf_release(struct dma_buf *dma_buf) -{ - struct drm_i915_gem_object *obj = dma_buf->priv; - - if (obj->base.export_dma_buf == dma_buf) { - /* drop the reference on the export fd holds */ - obj->base.export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(&obj->base); - } -} - static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) { struct drm_i915_gem_object *obj = dma_buf->priv; @@ -211,7 +200,7 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size static const struct dma_buf_ops i915_dmabuf_ops = { .map_dma_buf = i915_gem_map_dma_buf, .unmap_dma_buf = i915_gem_unmap_dma_buf, - .release = i915_gem_dmabuf_release, + .release = drm_gem_dmabuf_release, .kmap = i915_gem_dmabuf_kmap, .kmap_atomic = i915_gem_dmabuf_kmap_atomic, .kunmap = i915_gem_dmabuf_kunmap, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 4b518e0..cc991a2 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1537,6 +1537,7 @@ extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf); extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); +extern void drm_gem_dmabuf_release(struct dma_buf *dma_buf); extern int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 9:15 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter @ 2013-08-07 9:37 ` Chris Wilson 2013-08-07 9:40 ` Inki Dae 1 sibling, 0 replies; 30+ messages in thread From: Chris Wilson @ 2013-08-07 9:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 11:15:06AM +0200, Daniel Vetter wrote: > Note that this is slightly tricky since both drivers store their > native objects in dma_buf->priv. But both also embed the base > drm_gem_object at the first position, so the implicit cast is ok. > > To use the release helper we need to export it, too. > > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Any perverse driver could always call into drm_gem_dmabuf_release() with container_of(). Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 9:15 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter 2013-08-07 9:37 ` [Intel-gfx] " Chris Wilson @ 2013-08-07 9:40 ` Inki Dae 2013-08-07 9:55 ` Daniel Vetter 1 sibling, 1 reply; 30+ messages in thread From: Inki Dae @ 2013-08-07 9:40 UTC (permalink / raw) To: 'Daniel Vetter', 'DRI Development' Cc: 'Intel Graphics Development' > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > Sent: Wednesday, August 07, 2013 6:15 PM > To: DRI Development > Cc: Intel Graphics Development; Daniel Vetter; Inki Dae > Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos > drivers > > Note that this is slightly tricky since both drivers store their > native objects in dma_buf->priv. But both also embed the base > drm_gem_object at the first position, so the implicit cast is ok. > > To use the release helper we need to export it, too. Yeah, may I repost this patch with additional work? We also need to export with a gem object instead of specific one like you did. Thanks, Inki Dae > > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_prime.c | 3 ++- > drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 23 +---------------------- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 13 +------------ > include/drm/drmP.h | 1 + > 4 files changed, 5 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 85e450e..a35f206 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -192,7 +192,7 @@ static void drm_gem_unmap_dma_buf(struct > dma_buf_attachment *attach, > /* nothing to be done here */ > } > > -static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) > +void drm_gem_dmabuf_release(struct dma_buf *dma_buf) > { > struct drm_gem_object *obj = dma_buf->priv; > > @@ -202,6 +202,7 @@ static void drm_gem_dmabuf_release(struct dma_buf > *dma_buf) > drm_gem_object_unreference_unlocked(obj); > } > } > +EXPORT_SYMBOL(drm_gem_dmabuf_release); > > static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) > { > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > index a0f997e..3cd56e1 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > @@ -127,27 +127,6 @@ static void exynos_gem_unmap_dma_buf(struct > dma_buf_attachment *attach, > /* Nothing to do. */ > } > > -static void exynos_dmabuf_release(struct dma_buf *dmabuf) > -{ > - struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv; > - > - /* > - * exynos_dmabuf_release() call means that file object's > - * f_count is 0 and it calls drm_gem_object_handle_unreference() > - * to drop the references that these values had been increased > - * at drm_prime_handle_to_fd() > - */ > - if (exynos_gem_obj->base.export_dma_buf == dmabuf) { > - exynos_gem_obj->base.export_dma_buf = NULL; > - > - /* > - * drop this gem object refcount to release allocated buffer > - * and resources. > - */ > - drm_gem_object_unreference_unlocked(&exynos_gem_obj->base); > - } > -} > - > static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf, > unsigned long page_num) > { > @@ -193,7 +172,7 @@ static struct dma_buf_ops exynos_dmabuf_ops = { > .kunmap = exynos_gem_dmabuf_kunmap, > .kunmap_atomic = exynos_gem_dmabuf_kunmap_atomic, > .mmap = exynos_gem_dmabuf_mmap, > - .release = exynos_dmabuf_release, > + .release = drm_gem_dmabuf_release, > }; > > struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev, > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index f2e185c..63ee1a9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -90,17 +90,6 @@ static void i915_gem_unmap_dma_buf(struct > dma_buf_attachment *attachment, > kfree(sg); > } > > -static void i915_gem_dmabuf_release(struct dma_buf *dma_buf) > -{ > - struct drm_i915_gem_object *obj = dma_buf->priv; > - > - if (obj->base.export_dma_buf == dma_buf) { > - /* drop the reference on the export fd holds */ > - obj->base.export_dma_buf = NULL; > - drm_gem_object_unreference_unlocked(&obj->base); > - } > -} > - > static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > { > struct drm_i915_gem_object *obj = dma_buf->priv; > @@ -211,7 +200,7 @@ static int i915_gem_begin_cpu_access(struct dma_buf > *dma_buf, size_t start, size > static const struct dma_buf_ops i915_dmabuf_ops = { > .map_dma_buf = i915_gem_map_dma_buf, > .unmap_dma_buf = i915_gem_unmap_dma_buf, > - .release = i915_gem_dmabuf_release, > + .release = drm_gem_dmabuf_release, > .kmap = i915_gem_dmabuf_kmap, > .kmap_atomic = i915_gem_dmabuf_kmap_atomic, > .kunmap = i915_gem_dmabuf_kunmap, > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 4b518e0..cc991a2 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1537,6 +1537,7 @@ extern struct drm_gem_object > *drm_gem_prime_import(struct drm_device *dev, > struct dma_buf *dma_buf); > extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, > struct drm_file *file_priv, int prime_fd, uint32_t *handle); > +extern void drm_gem_dmabuf_release(struct dma_buf *dma_buf); > > extern int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void > *data, > struct drm_file *file_priv); > -- > 1.8.3.2 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 9:40 ` Inki Dae @ 2013-08-07 9:55 ` Daniel Vetter 2013-08-07 10:18 ` Joonyoung Shim 0 siblings, 1 reply; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 9:55 UTC (permalink / raw) To: Inki Dae; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote: >> -----Original Message----- >> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] >> Sent: Wednesday, August 07, 2013 6:15 PM >> To: DRI Development >> Cc: Intel Graphics Development; Daniel Vetter; Inki Dae >> Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos >> drivers >> >> Note that this is slightly tricky since both drivers store their >> native objects in dma_buf->priv. But both also embed the base >> drm_gem_object at the first position, so the implicit cast is ok. >> >> To use the release helper we need to export it, too. > > Yeah, may I repost this patch with additional work? We also need to export > with a gem object instead of specific one like you did. I'm confused here what you mean, so pls just submit the patch. That usually helps ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 9:55 ` Daniel Vetter @ 2013-08-07 10:18 ` Joonyoung Shim 2013-08-07 10:21 ` Daniel Vetter 0 siblings, 1 reply; 30+ messages in thread From: Joonyoung Shim @ 2013-08-07 10:18 UTC (permalink / raw) To: Inki Dae; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development On 08/07/2013 06:55 PM, Daniel Vetter wrote: > On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote: >>> -----Original Message----- >>> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] >>> Sent: Wednesday, August 07, 2013 6:15 PM >>> To: DRI Development >>> Cc: Intel Graphics Development; Daniel Vetter; Inki Dae >>> Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos >>> drivers >>> >>> Note that this is slightly tricky since both drivers store their >>> native objects in dma_buf->priv. But both also embed the base >>> drm_gem_object at the first position, so the implicit cast is ok. >>> >>> To use the release helper we need to export it, too. >> Yeah, may I repost this patch with additional work? We also need to export >> with a gem object instead of specific one like you did. I think dmabuf stuff of exynos can be replaced to common drm_gem_dmabuf. Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common drm_gem_dmabuf with low-level hook functions to use prime helpers. Thanks. > I'm confused here what you mean, so pls just submit the patch. That > usually helps ;-) > -Daniel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 10:18 ` Joonyoung Shim @ 2013-08-07 10:21 ` Daniel Vetter 2013-08-07 10:29 ` Joonyoung Shim 2013-08-07 12:01 ` Inki Dae 0 siblings, 2 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 10:21 UTC (permalink / raw) To: Joonyoung Shim; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: > On 08/07/2013 06:55 PM, Daniel Vetter wrote: > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote: > >>>-----Original Message----- > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > >>>Sent: Wednesday, August 07, 2013 6:15 PM > >>>To: DRI Development > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos > >>>drivers > >>> > >>>Note that this is slightly tricky since both drivers store their > >>>native objects in dma_buf->priv. But both also embed the base > >>>drm_gem_object at the first position, so the implicit cast is ok. > >>> > >>>To use the release helper we need to export it, too. > >>Yeah, may I repost this patch with additional work? We also need to export > >>with a gem object instead of specific one like you did. > > I think dmabuf stuff of exynos can be replaced to common drm_gem_dmabuf. > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common > drm_gem_dmabuf with low-level hook functions to use prime helpers. Ah, but that can easily be done on top of this, right? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 10:21 ` Daniel Vetter @ 2013-08-07 10:29 ` Joonyoung Shim 2013-08-07 12:01 ` Inki Dae 1 sibling, 0 replies; 30+ messages in thread From: Joonyoung Shim @ 2013-08-07 10:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development On 08/07/2013 07:21 PM, Daniel Vetter wrote: > On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: >> On 08/07/2013 06:55 PM, Daniel Vetter wrote: >>> On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote: >>>>> -----Original Message----- >>>>> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] >>>>> Sent: Wednesday, August 07, 2013 6:15 PM >>>>> To: DRI Development >>>>> Cc: Intel Graphics Development; Daniel Vetter; Inki Dae >>>>> Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos >>>>> drivers >>>>> >>>>> Note that this is slightly tricky since both drivers store their >>>>> native objects in dma_buf->priv. But both also embed the base >>>>> drm_gem_object at the first position, so the implicit cast is ok. >>>>> >>>>> To use the release helper we need to export it, too. >>>> Yeah, may I repost this patch with additional work? We also need to export >>>> with a gem object instead of specific one like you did. >> I think dmabuf stuff of exynos can be replaced to common drm_gem_dmabuf. >> Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common >> drm_gem_dmabuf with low-level hook functions to use prime helpers. > Ah, but that can easily be done on top of this, right? > -Daniel I think it doesn't matter. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 10:21 ` Daniel Vetter 2013-08-07 10:29 ` Joonyoung Shim @ 2013-08-07 12:01 ` Inki Dae 2013-08-07 12:07 ` Daniel Vetter 1 sibling, 1 reply; 30+ messages in thread From: Inki Dae @ 2013-08-07 12:01 UTC (permalink / raw) To: Daniel Vetter Cc: Daniel Vetter, Intel Graphics Development, Joonyoung Shim, DRI Development [-- Attachment #1.1: Type: text/plain, Size: 2397 bytes --] 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: > > On 08/07/2013 06:55 PM, Daniel Vetter wrote: > > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote: > > >>>-----Original Message----- > > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > > >>>Sent: Wednesday, August 07, 2013 6:15 PM > > >>>To: DRI Development > > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae > > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in > i915/exynos > > >>>drivers > > >>> > > >>>Note that this is slightly tricky since both drivers store their > > >>>native objects in dma_buf->priv. But both also embed the base > > >>>drm_gem_object at the first position, so the implicit cast is ok. > > >>> > > >>>To use the release helper we need to export it, too. > > >>Yeah, may I repost this patch with additional work? We also need to > export > > >>with a gem object instead of specific one like you did. > > > > I think dmabuf stuff of exynos can be replaced to common drm_gem_dmabuf. > > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common > > drm_gem_dmabuf with low-level hook functions to use prime helpers. > > Ah, but that can easily be done on top of this, right? > Daniel, could you remove exynos related codes from your patch set? Your patch set would make exynos broken because you didn't consider exporting with a gem object for exynos like [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv. So I think your patch set is not complete set, and That is why exynos needs the additional work I mentioned above. So I just wanted to repost your patch set + new one. However, I think not only exynos could go to common drm_gem_dmabuf directly but also it would make your patch set to be complete set if you remove exynos related codes from your patch set. Otherwise, we have to work twice. one is the additional work for resolving exynos broken issue by your patch set, and other is to replace existing dmabuf stuff of exynos to common drm_gem_dmabuf. Thanks, Inki Dae > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > [-- Attachment #1.2: Type: text/html, Size: 3835 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 12:01 ` Inki Dae @ 2013-08-07 12:07 ` Daniel Vetter 2013-08-07 12:37 ` Inki Dae 0 siblings, 1 reply; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 12:07 UTC (permalink / raw) To: Inki Dae; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae <inki.dae@samsung.com> wrote: > > > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> >> >> On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: >> > On 08/07/2013 06:55 PM, Daniel Vetter wrote: >> > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote: >> > >>>-----Original Message----- >> > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] >> > >>>Sent: Wednesday, August 07, 2013 6:15 PM >> > >>>To: DRI Development >> > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae >> > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in >> > >>> i915/exynos >> > >>>drivers >> > >>> >> > >>>Note that this is slightly tricky since both drivers store their >> > >>>native objects in dma_buf->priv. But both also embed the base >> > >>>drm_gem_object at the first position, so the implicit cast is ok. >> > >>> >> > >>>To use the release helper we need to export it, too. >> > >>Yeah, may I repost this patch with additional work? We also need to >> > >> export >> > >>with a gem object instead of specific one like you did. >> > >> > I think dmabuf stuff of exynos can be replaced to common drm_gem_dmabuf. >> > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common >> > drm_gem_dmabuf with low-level hook functions to use prime helpers. >> >> Ah, but that can easily be done on top of this, right? > > > Daniel, could you remove exynos related codes from your patch set? Your > patch set would make exynos broken because you didn't consider exporting > with a gem object for exynos like [PATCH 3/3] drm/i915: explicit store base > gem object in dma_buf->priv. So I think your patch set is not complete set, > and That is why exynos needs the additional work I mentioned above. So I > just wanted to repost your patch set + new one. Nope, my patch should not break exynos since the base gem_object is the first member of the exynos object, so we don't have any issues with upcasting in exynos dma-buf code. The same applies to i915 dma-buf code, my follow-up patch just makes the code a bit safer. > However, I think not only exynos could go to common drm_gem_dmabuf directly > but also it would make your patch set to be complete set if you remove > exynos related codes from your patch set. Otherwise, we have to work twice. > one is the additional work for resolving exynos broken issue by your patch > set, and other is to replace existing dmabuf stuff of exynos to common > drm_gem_dmabuf. Yeah np, I'll drop exynos then. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 12:07 ` Daniel Vetter @ 2013-08-07 12:37 ` Inki Dae 2013-08-07 23:21 ` Daniel Vetter 0 siblings, 1 reply; 30+ messages in thread From: Inki Dae @ 2013-08-07 12:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development [-- Attachment #1.1: Type: text/plain, Size: 3082 bytes --] 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae <inki.dae@samsung.com> wrote: > > > > > > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > >> > >> On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: > >> > On 08/07/2013 06:55 PM, Daniel Vetter wrote: > >> > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> > wrote: > >> > >>>-----Original Message----- > >> > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > >> > >>>Sent: Wednesday, August 07, 2013 6:15 PM > >> > >>>To: DRI Development > >> > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae > >> > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in > >> > >>> i915/exynos > >> > >>>drivers > >> > >>> > >> > >>>Note that this is slightly tricky since both drivers store their > >> > >>>native objects in dma_buf->priv. But both also embed the base > >> > >>>drm_gem_object at the first position, so the implicit cast is ok. > >> > >>> > >> > >>>To use the release helper we need to export it, too. > >> > >>Yeah, may I repost this patch with additional work? We also need to > >> > >> export > >> > >>with a gem object instead of specific one like you did. > >> > > >> > I think dmabuf stuff of exynos can be replaced to common > drm_gem_dmabuf. > >> > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common > >> > drm_gem_dmabuf with low-level hook functions to use prime helpers. > >> > >> Ah, but that can easily be done on top of this, right? > > > > > > Daniel, could you remove exynos related codes from your patch set? Your > > patch set would make exynos broken because you didn't consider exporting > > with a gem object for exynos like [PATCH 3/3] drm/i915: explicit store > base > > gem object in dma_buf->priv. So I think your patch set is not complete > set, > > and That is why exynos needs the additional work I mentioned above. So I > > just wanted to repost your patch set + new one. > > Nope, my patch should not break exynos since the base gem_object is > the first member of the exynos object, so we don't have any issues > Ah, right. However, it does not seem like good way. > with upcasting in exynos dma-buf code. The same applies to i915 > dma-buf code, my follow-up patch just makes the code a bit safer. > > > > > > However, I think not only exynos could go to common drm_gem_dmabuf > directly > > but also it would make your patch set to be complete set if you remove > > exynos related codes from your patch set. Otherwise, we have to work > twice. > > one is the additional work for resolving exynos broken issue by your > patch > > set, and other is to replace existing dmabuf stuff of exynos to common > > drm_gem_dmabuf. > > Yeah np, I'll drop exynos then. > Thanks a lot. :) Thanks, Inki Dae > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > [-- Attachment #1.2: Type: text/html, Size: 5538 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 12:37 ` Inki Dae @ 2013-08-07 23:21 ` Daniel Vetter 2013-08-08 4:32 ` Inki Dae 0 siblings, 1 reply; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 23:21 UTC (permalink / raw) To: Inki Dae; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 09:37:52PM +0900, Inki Dae wrote: > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > > > On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae <inki.dae@samsung.com> wrote: > > > > > > > > > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > > >> > > >> On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: > > >> > On 08/07/2013 06:55 PM, Daniel Vetter wrote: > > >> > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> > > wrote: > > >> > >>>-----Original Message----- > > >> > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > > >> > >>>Sent: Wednesday, August 07, 2013 6:15 PM > > >> > >>>To: DRI Development > > >> > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae > > >> > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in > > >> > >>> i915/exynos > > >> > >>>drivers > > >> > >>> > > >> > >>>Note that this is slightly tricky since both drivers store their > > >> > >>>native objects in dma_buf->priv. But both also embed the base > > >> > >>>drm_gem_object at the first position, so the implicit cast is ok. > > >> > >>> > > >> > >>>To use the release helper we need to export it, too. > > >> > >>Yeah, may I repost this patch with additional work? We also need to > > >> > >> export > > >> > >>with a gem object instead of specific one like you did. > > >> > > > >> > I think dmabuf stuff of exynos can be replaced to common > > drm_gem_dmabuf. > > >> > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to common > > >> > drm_gem_dmabuf with low-level hook functions to use prime helpers. > > >> > > >> Ah, but that can easily be done on top of this, right? > > > > > > > > > Daniel, could you remove exynos related codes from your patch set? Your > > > patch set would make exynos broken because you didn't consider exporting > > > with a gem object for exynos like [PATCH 3/3] drm/i915: explicit store > > base > > > gem object in dma_buf->priv. So I think your patch set is not complete > > set, > > > and That is why exynos needs the additional work I mentioned above. So I > > > just wanted to repost your patch set + new one. > > > > Nope, my patch should not break exynos since the base gem_object is > > the first member of the exynos object, so we don't have any issues > > > > Ah, right. However, it does not seem like good way. > > > > with upcasting in exynos dma-buf code. The same applies to i915 > > dma-buf code, my follow-up patch just makes the code a bit safer. > > > > > > > > > > > > > > However, I think not only exynos could go to common drm_gem_dmabuf > > directly > > > but also it would make your patch set to be complete set if you remove > > > exynos related codes from your patch set. Otherwise, we have to work > > twice. > > > one is the additional work for resolving exynos broken issue by your > > patch > > > set, and other is to replace existing dmabuf stuff of exynos to common > > > drm_gem_dmabuf. > > > > Yeah np, I'll drop exynos then. > > > > Thanks a lot. :) Ah, I remember again why I want to also convert over exynos to the common dma buf release function: Later patches in my prime locking series will change things in there to avoid a userspace-triggerable oops. If we leave out exynos it'll break rather badly for dma-buf export. I need to think a bit more about what stuff looks like atm, but if I resend those parts I'll include exynos. It's a bit tricky that it still works, but that way you can fix it up without the introduction of a bisect failure point. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-07 23:21 ` Daniel Vetter @ 2013-08-08 4:32 ` Inki Dae 2013-08-08 4:39 ` [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv Inki Dae 2013-08-08 6:31 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter 0 siblings, 2 replies; 30+ messages in thread From: Inki Dae @ 2013-08-08 4:32 UTC (permalink / raw) To: 'Daniel Vetter' Cc: 'Intel Graphics Development', 'DRI Development' > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Thursday, August 08, 2013 8:21 AM > To: Inki Dae > Cc: Daniel Vetter; Intel Graphics Development; DRI Development > Subject: Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in > i915/exynos drivers > > On Wed, Aug 07, 2013 at 09:37:52PM +0900, Inki Dae wrote: > > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > > > > > On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae <inki.dae@samsung.com> wrote: > > > > > > > > > > > > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> > > > >> > > > >> On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: > > > >> > On 08/07/2013 06:55 PM, Daniel Vetter wrote: > > > >> > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> > > > wrote: > > > >> > >>>-----Original Message----- > > > >> > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > > > >> > >>>Sent: Wednesday, August 07, 2013 6:15 PM > > > >> > >>>To: DRI Development > > > >> > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae > > > >> > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in > > > >> > >>> i915/exynos > > > >> > >>>drivers > > > >> > >>> > > > >> > >>>Note that this is slightly tricky since both drivers store > their > > > >> > >>>native objects in dma_buf->priv. But both also embed the base > > > >> > >>>drm_gem_object at the first position, so the implicit cast is > ok. > > > >> > >>> > > > >> > >>>To use the release helper we need to export it, too. > > > >> > >>Yeah, may I repost this patch with additional work? We also > need to > > > >> > >> export > > > >> > >>with a gem object instead of specific one like you did. > > > >> > > > > >> > I think dmabuf stuff of exynos can be replaced to common > > > drm_gem_dmabuf. > > > >> > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to > common > > > >> > drm_gem_dmabuf with low-level hook functions to use prime helpers. > > > >> > > > >> Ah, but that can easily be done on top of this, right? > > > > > > > > > > > > Daniel, could you remove exynos related codes from your patch set? > Your > > > > patch set would make exynos broken because you didn't consider > exporting > > > > with a gem object for exynos like [PATCH 3/3] drm/i915: explicit > store > > > base > > > > gem object in dma_buf->priv. So I think your patch set is not > complete > > > set, > > > > and That is why exynos needs the additional work I mentioned above. > So I > > > > just wanted to repost your patch set + new one. > > > > > > Nope, my patch should not break exynos since the base gem_object is > > > the first member of the exynos object, so we don't have any issues > > > > > > > Ah, right. However, it does not seem like good way. > > > > > > > with upcasting in exynos dma-buf code. The same applies to i915 > > > dma-buf code, my follow-up patch just makes the code a bit safer. > > > > > > > > > > > > > > > > > > > > > > However, I think not only exynos could go to common drm_gem_dmabuf > > > directly > > > > but also it would make your patch set to be complete set if you > remove > > > > exynos related codes from your patch set. Otherwise, we have to work > > > twice. > > > > one is the additional work for resolving exynos broken issue by your > > > patch > > > > set, and other is to replace existing dmabuf stuff of exynos to > common > > > > drm_gem_dmabuf. > > > > > > Yeah np, I'll drop exynos then. > > > > > > > Thanks a lot. :) > > Ah, I remember again why I want to also convert over exynos to the common > dma buf release function: Later patches in my prime locking series will > change things in there to avoid a userspace-triggerable oops. If we leave > out exynos it'll break rather badly for dma-buf export. > > I need to think a bit more about what stuff looks like atm, but if I > resend those parts I'll include exynos. It's a bit tricky that it still > works, but that way you can fix it up without the introduction of a bisect > failure point. I'll repost your patch set + new one that exports to a common gem object; already worked and tested. I think it's good for they to be one set because only using the patch 1/3 doesn't look good even though Exynos works fine with the path 1/3. So I'll repost it like below if you agree with me, [PATCH 0/4] Small i915/exynos prime cleanup [PATCH 1/4] drm: use common drm_gem_dmabuf_release in i915/exynos drivers [PATCH 2/4] drm/i915: unpin backing storage in dmabuf_unmap [PATCH 3/4] drm/i915: explicit store base gem object in dma_buf->priv [PATCH 4/4] drm/exynos: explicit store base gem object in dma_buf->priv After this, you can take care of them until merged to next. Or you can repost this patch set including my patch again. What you proper doesn't matter to me. :) And it seems better that exynos keeps using existing dmabuf interfaces without replacing them to common drm_gem_dmabuf ones because we may need features only for Exynos. Actually, now exynos dmabuf includes some features related to v4l2 and gpu driver for more performance. Thanks, Inki Dae > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv 2013-08-08 4:32 ` Inki Dae @ 2013-08-08 4:39 ` Inki Dae 2013-08-08 4:56 ` Inki Dae 2013-08-08 6:31 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter 1 sibling, 1 reply; 30+ messages in thread From: Inki Dae @ 2013-08-08 4:39 UTC (permalink / raw) To: daniel; +Cc: Inki Dae, intel-gfx, Kyungmin Park, dri-devel Signed-off-by: Inki Dae <inki.dae@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index 3cd56e1..fd76449 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -22,6 +22,11 @@ struct exynos_drm_dmabuf_attachment { bool is_mapped; }; +static struct exynos_drm_gem_obj *dma_buf_to_obj(struct dma_buf *buf) +{ + return to_exynos_gem_obj(buf->priv); +} + static int exynos_gem_attach_dma_buf(struct dma_buf *dmabuf, struct device *dev, struct dma_buf_attachment *attach) @@ -63,7 +68,7 @@ static struct sg_table * enum dma_data_direction dir) { struct exynos_drm_dmabuf_attachment *exynos_attach = attach->priv; - struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv; + struct exynos_drm_gem_obj *gem_obj = dma_buf_to_obj(attach->dmabuf); struct drm_device *dev = gem_obj->base.dev; struct exynos_drm_gem_buf *buf; struct scatterlist *rd, *wr; @@ -180,7 +185,7 @@ struct dma_buf *exynos_dmabuf_prime_export(struct drm_device *drm_dev, { struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); - return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, + return dma_buf_export(obj, &exynos_dmabuf_ops, exynos_gem_obj->base.size, flags); } @@ -198,8 +203,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev, if (dma_buf->ops == &exynos_dmabuf_ops) { struct drm_gem_object *obj; - exynos_gem_obj = dma_buf->priv; - obj = &exynos_gem_obj->base; + obj = dma_buf->priv; /* is it from our device? */ if (obj->dev == drm_dev) { -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* RE: [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv 2013-08-08 4:39 ` [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv Inki Dae @ 2013-08-08 4:56 ` Inki Dae 2013-08-08 6:58 ` Daniel Vetter 0 siblings, 1 reply; 30+ messages in thread From: Inki Dae @ 2013-08-08 4:56 UTC (permalink / raw) To: 'Inki Dae', daniel; +Cc: intel-gfx, 'Kyungmin Park', dri-devel Hi, Daniel. You can repost your patch set including the below my patch. And my patch numbering is wrong. Sorry about that. [PATCH 1/4] -> [PATCH 4/4] Thanks, Inki Dae > -----Original Message----- > From: Inki Dae [mailto:inki.dae@samsung.com] > Sent: Thursday, August 08, 2013 1:40 PM > To: daniel@ffwll.ch > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Inki > Dae; Kyungmin Park > Subject: [PATCH 1/4] drm/exynos: explicit store base gem object in > dma_buf->priv > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 12 ++++++++---- > 1 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > index 3cd56e1..fd76449 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > @@ -22,6 +22,11 @@ struct exynos_drm_dmabuf_attachment { > bool is_mapped; > }; > > +static struct exynos_drm_gem_obj *dma_buf_to_obj(struct dma_buf *buf) > +{ > + return to_exynos_gem_obj(buf->priv); > +} > + > static int exynos_gem_attach_dma_buf(struct dma_buf *dmabuf, > struct device *dev, > struct dma_buf_attachment *attach) > @@ -63,7 +68,7 @@ static struct sg_table * > enum dma_data_direction dir) > { > struct exynos_drm_dmabuf_attachment *exynos_attach = attach->priv; > - struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv; > + struct exynos_drm_gem_obj *gem_obj = dma_buf_to_obj(attach->dmabuf); > struct drm_device *dev = gem_obj->base.dev; > struct exynos_drm_gem_buf *buf; > struct scatterlist *rd, *wr; > @@ -180,7 +185,7 @@ struct dma_buf *exynos_dmabuf_prime_export(struct > drm_device *drm_dev, > { > struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); > > - return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, > + return dma_buf_export(obj, &exynos_dmabuf_ops, > exynos_gem_obj->base.size, flags); > } > > @@ -198,8 +203,7 @@ struct drm_gem_object > *exynos_dmabuf_prime_import(struct drm_device *drm_dev, > if (dma_buf->ops == &exynos_dmabuf_ops) { > struct drm_gem_object *obj; > > - exynos_gem_obj = dma_buf->priv; > - obj = &exynos_gem_obj->base; > + obj = dma_buf->priv; > > /* is it from our device? */ > if (obj->dev == drm_dev) { > -- > 1.7.5.4 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv 2013-08-08 4:56 ` Inki Dae @ 2013-08-08 6:58 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-08 6:58 UTC (permalink / raw) To: Inki Dae; +Cc: 'Kyungmin Park', intel-gfx, dri-devel Hi Inki, On Thu, Aug 08, 2013 at 01:56:28PM +0900, Inki Dae wrote: > Hi, Daniel. You can repost your patch set including the below my patch. And > my patch numbering is wrong. Sorry about that. Thanks for the patch, I'll submit it toghether with the other ones soon. -Daniel > > [PATCH 1/4] -> [PATCH 4/4] > > > Thanks, > Inki Dae > > > -----Original Message----- > > From: Inki Dae [mailto:inki.dae@samsung.com] > > Sent: Thursday, August 08, 2013 1:40 PM > > To: daniel@ffwll.ch > > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Inki > > Dae; Kyungmin Park > > Subject: [PATCH 1/4] drm/exynos: explicit store base gem object in > > dma_buf->priv > > > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > --- > > drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 12 ++++++++---- > > 1 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > > b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > > index 3cd56e1..fd76449 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c > > @@ -22,6 +22,11 @@ struct exynos_drm_dmabuf_attachment { > > bool is_mapped; > > }; > > > > +static struct exynos_drm_gem_obj *dma_buf_to_obj(struct dma_buf *buf) > > +{ > > + return to_exynos_gem_obj(buf->priv); > > +} > > + > > static int exynos_gem_attach_dma_buf(struct dma_buf *dmabuf, > > struct device *dev, > > struct dma_buf_attachment *attach) > > @@ -63,7 +68,7 @@ static struct sg_table * > > enum dma_data_direction dir) > > { > > struct exynos_drm_dmabuf_attachment *exynos_attach = attach->priv; > > - struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv; > > + struct exynos_drm_gem_obj *gem_obj = dma_buf_to_obj(attach->dmabuf); > > struct drm_device *dev = gem_obj->base.dev; > > struct exynos_drm_gem_buf *buf; > > struct scatterlist *rd, *wr; > > @@ -180,7 +185,7 @@ struct dma_buf *exynos_dmabuf_prime_export(struct > > drm_device *drm_dev, > > { > > struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); > > > > - return dma_buf_export(exynos_gem_obj, &exynos_dmabuf_ops, > > + return dma_buf_export(obj, &exynos_dmabuf_ops, > > exynos_gem_obj->base.size, flags); > > } > > > > @@ -198,8 +203,7 @@ struct drm_gem_object > > *exynos_dmabuf_prime_import(struct drm_device *drm_dev, > > if (dma_buf->ops == &exynos_dmabuf_ops) { > > struct drm_gem_object *obj; > > > > - exynos_gem_obj = dma_buf->priv; > > - obj = &exynos_gem_obj->base; > > + obj = dma_buf->priv; > > > > /* is it from our device? */ > > if (obj->dev == drm_dev) { > > -- > > 1.7.5.4 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers 2013-08-08 4:32 ` Inki Dae 2013-08-08 4:39 ` [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv Inki Dae @ 2013-08-08 6:31 ` Daniel Vetter 1 sibling, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-08 6:31 UTC (permalink / raw) To: Inki Dae; +Cc: Intel Graphics Development, DRI Development On Thu, Aug 8, 2013 at 6:32 AM, Inki Dae <inki.dae@samsung.com> wrote: > > >> -----Original Message----- >> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel >> Vetter >> Sent: Thursday, August 08, 2013 8:21 AM >> To: Inki Dae >> Cc: Daniel Vetter; Intel Graphics Development; DRI Development >> Subject: Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in >> i915/exynos drivers >> >> On Wed, Aug 07, 2013 at 09:37:52PM +0900, Inki Dae wrote: >> > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> >> > >> > > On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae <inki.dae@samsung.com> wrote: >> > > > >> > > > >> > > > 2013/8/7 Daniel Vetter <daniel@ffwll.ch> >> > > >> >> > > >> On Wed, Aug 07, 2013 at 07:18:45PM +0900, Joonyoung Shim wrote: >> > > >> > On 08/07/2013 06:55 PM, Daniel Vetter wrote: >> > > >> > >On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae <inki.dae@samsung.com> >> > > wrote: >> > > >> > >>>-----Original Message----- >> > > >> > >>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] >> > > >> > >>>Sent: Wednesday, August 07, 2013 6:15 PM >> > > >> > >>>To: DRI Development >> > > >> > >>>Cc: Intel Graphics Development; Daniel Vetter; Inki Dae >> > > >> > >>>Subject: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in >> > > >> > >>> i915/exynos >> > > >> > >>>drivers >> > > >> > >>> >> > > >> > >>>Note that this is slightly tricky since both drivers store >> their >> > > >> > >>>native objects in dma_buf->priv. But both also embed the base >> > > >> > >>>drm_gem_object at the first position, so the implicit cast is >> ok. >> > > >> > >>> >> > > >> > >>>To use the release helper we need to export it, too. >> > > >> > >>Yeah, may I repost this patch with additional work? We also >> need to >> > > >> > >> export >> > > >> > >>with a gem object instead of specific one like you did. >> > > >> > >> > > >> > I think dmabuf stuff of exynos can be replaced to common >> > > drm_gem_dmabuf. >> > > >> > Already dmabuf stuff of drm_gem_cma_helper.c was substituted to >> common >> > > >> > drm_gem_dmabuf with low-level hook functions to use prime > helpers. >> > > >> >> > > >> Ah, but that can easily be done on top of this, right? >> > > > >> > > > >> > > > Daniel, could you remove exynos related codes from your patch set? >> Your >> > > > patch set would make exynos broken because you didn't consider >> exporting >> > > > with a gem object for exynos like [PATCH 3/3] drm/i915: explicit >> store >> > > base >> > > > gem object in dma_buf->priv. So I think your patch set is not >> complete >> > > set, >> > > > and That is why exynos needs the additional work I mentioned above. >> So I >> > > > just wanted to repost your patch set + new one. >> > > >> > > Nope, my patch should not break exynos since the base gem_object is >> > > the first member of the exynos object, so we don't have any issues >> > > >> > >> > Ah, right. However, it does not seem like good way. >> > >> > >> > > with upcasting in exynos dma-buf code. The same applies to i915 >> > > dma-buf code, my follow-up patch just makes the code a bit safer. >> > > >> > > >> > > >> > >> > > >> > >> > > >> > > However, I think not only exynos could go to common drm_gem_dmabuf >> > > directly >> > > > but also it would make your patch set to be complete set if you >> remove >> > > > exynos related codes from your patch set. Otherwise, we have to work >> > > twice. >> > > > one is the additional work for resolving exynos broken issue by your >> > > patch >> > > > set, and other is to replace existing dmabuf stuff of exynos to >> common >> > > > drm_gem_dmabuf. >> > > >> > > Yeah np, I'll drop exynos then. >> > > >> > >> > Thanks a lot. :) >> >> Ah, I remember again why I want to also convert over exynos to the common >> dma buf release function: Later patches in my prime locking series will >> change things in there to avoid a userspace-triggerable oops. If we leave >> out exynos it'll break rather badly for dma-buf export. >> >> I need to think a bit more about what stuff looks like atm, but if I >> resend those parts I'll include exynos. It's a bit tricky that it still >> works, but that way you can fix it up without the introduction of a bisect >> failure point. > > I'll repost your patch set + new one that exports to a common gem object; > already worked and tested. I think it's good for they to be one set because > only using the patch 1/3 doesn't look good even though Exynos works fine > with the path 1/3. > > So I'll repost it like below if you agree with me, > [PATCH 0/4] Small i915/exynos prime cleanup > [PATCH 1/4] drm: use common drm_gem_dmabuf_release in i915/exynos drivers > [PATCH 2/4] drm/i915: unpin backing storage in dmabuf_unmap > [PATCH 3/4] drm/i915: explicit store base gem object in dma_buf->priv > [PATCH 4/4] drm/exynos: explicit store base gem object in dma_buf->priv > > After this, you can take care of them until merged to next. Or you can > repost this patch set including my patch again. What you proper doesn't > matter to me. :) Yeah, sounds like a plan. And I think those 4 patches can go in earlier, the later patches I have need some more thought. Note that the i915 patches have new versions meanwhile, so if you just submit the exynos one I can integrate into my series. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 9:15 [PATCH 0/3] Small i915/exynos prime cleanup Daniel Vetter 2013-08-07 9:15 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter @ 2013-08-07 9:15 ` Daniel Vetter 2013-08-07 9:19 ` Daniel Vetter 2013-08-07 9:29 ` [Intel-gfx] " Chris Wilson 2013-08-07 9:15 ` [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv Daniel Vetter 2 siblings, 2 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 9:15 UTC (permalink / raw) To: DRI Development Cc: Maarten Lankhorst, Daniel Vetter, Intel Graphics Development This fixes a WARN in i915_gem_free_object when the obj->pages_pin_count isn't 0. Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 63ee1a9..0bf3d51 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -85,9 +85,13 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { + struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); kfree(sg); + + i915_gem_object_unpin_pages(obj); } static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 9:15 ` [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap Daniel Vetter @ 2013-08-07 9:19 ` Daniel Vetter 2013-08-07 9:29 ` [Intel-gfx] " Chris Wilson 1 sibling, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 9:19 UTC (permalink / raw) To: DRI Development Cc: Maarten Lankhorst, Daniel Vetter, Intel Graphics Development On Wed, Aug 07, 2013 at 11:15:07AM +0200, Daniel Vetter wrote: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67391 > --- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index 63ee1a9..0bf3d51 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -85,9 +85,13 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, > struct sg_table *sg, > enum dma_data_direction dir) > { > + struct drm_i915_gem_object *obj = attachment->dmabuf->priv; > + > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); > sg_free_table(sg); > kfree(sg); > + > + i915_gem_object_unpin_pages(obj); > } > > static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > -- > 1.8.3.2 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 9:15 ` [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap Daniel Vetter 2013-08-07 9:19 ` Daniel Vetter @ 2013-08-07 9:29 ` Chris Wilson 2013-08-07 9:49 ` Daniel Vetter 1 sibling, 1 reply; 30+ messages in thread From: Chris Wilson @ 2013-08-07 9:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 11:15:07AM +0200, Daniel Vetter wrote: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Papers over the WARN with iffy locking. Not all callers hold struct_mutex, right? Worse some do, some don't... What's the long term plan here? -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 9:29 ` [Intel-gfx] " Chris Wilson @ 2013-08-07 9:49 ` Daniel Vetter 2013-08-07 10:09 ` [PATCH 1/2] " Daniel Vetter 0 siblings, 1 reply; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 9:49 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, DRI Development, Maarten Lankhorst, Intel Graphics Development On Wed, Aug 7, 2013 at 11:29 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Aug 07, 2013 at 11:15:07AM +0200, Daniel Vetter wrote: >> This fixes a WARN in i915_gem_free_object when the >> obj->pages_pin_count isn't 0. >> >> Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Papers over the WARN with iffy locking. Not all callers hold > struct_mutex, right? Worse some do, some don't... Oops, that needs mutex locking here. I've checked the code and none of the callers here should ever hold our own dev->struct_mutex (due to the self-import checks we bypass dma-buf for our own objects) so no immediate deadlock. But it's easy to create circles and piss off lockded ofc. > What's the long term plan here? Per-bo locking with ww mutexes. Locking is done by the callers of map/unmap, sonce only those can properly do the ww locking dance on all relevent buffers of a batch upfront. It's going to be fun ;-) Looking closer I've also spotted that our map_buf callback has a call to i915_mutex_lock_interruptible, which means the map can fail with -EINTR. Currently it seems unspec'ed whether map is allowed to do that, but current callers certainly can't cope with this. I'll throw a 2nd patch on top. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 9:49 ` Daniel Vetter @ 2013-08-07 10:09 ` Daniel Vetter 2013-08-07 10:09 ` [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map Daniel Vetter ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 10:09 UTC (permalink / raw) To: DRI Development Cc: Maarten Lankhorst, Daniel Vetter, Intel Graphics Development This fixes a WARN in i915_gem_free_object when the obj->pages_pin_count isn't 0. v2: Add locking to unmap, noticed by Chris Wilson. Note that even though we call unmap with our own dev->struct_mutex held that won't result in an immediate deadlock since we never go through the dma_buf interfaces for our own, reimported buffers. But it's still easy to blow up and anger lockdep, but that's already the case with our ->map implementation. Fixing this for real will involve per dma-buf ww mutex locking by the callers. And lots of fun. So go with the duct-tape approach for now. Cc: Chris Wilson <chris@chris-wilson.co.uk> Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> Tested-by: Armin K. <krejzi@email.com> (v1) Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 63ee1a9..f7e1682 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { + struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + + mutex_lock(&obj->base.dev->struct_mutex); + dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); kfree(sg); + + i915_gem_object_unpin_pages(obj); + + mutex_unlock(&obj->base.dev->struct_mutex); } static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map 2013-08-07 10:09 ` [PATCH 1/2] " Daniel Vetter @ 2013-08-07 10:09 ` Daniel Vetter 2013-08-07 10:30 ` Chris Wilson 2013-08-07 10:40 ` [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap Maarten Lankhorst 2013-08-08 0:50 ` Konrad Rzeszutek Wilk 2 siblings, 1 reply; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 10:09 UTC (permalink / raw) To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development It's unclear whether ->map is allowed to fail with -EINTR, but looking at current callers it's pretty clear that they don't expect this to happen. So use a blocking mutex_lock call. Since we don't wait for the gpu in our ->map callback the lack of the gpu hang checks doesn't matter. Furthermore the goal is to eventually have per dma-buf locking done by callers with ww mutexes, so this will then be removed. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index f7e1682..63c0818 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -35,9 +35,7 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i; - ret = i915_mutex_lock_interruptible(obj->base.dev); - if (ret) - return ERR_PTR(ret); + mutex_lock(&obj->base.dev->struct_mutex); ret = i915_gem_object_get_pages(obj); if (ret) { -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map 2013-08-07 10:09 ` [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map Daniel Vetter @ 2013-08-07 10:30 ` Chris Wilson 2013-08-07 10:49 ` Daniel Vetter 0 siblings, 1 reply; 30+ messages in thread From: Chris Wilson @ 2013-08-07 10:30 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 12:09:33PM +0200, Daniel Vetter wrote: > It's unclear whether ->map is allowed to fail with -EINTR, but > looking at current callers it's pretty clear that they don't > expect this to happen. So use a blocking mutex_lock call. Since > we don't wait for the gpu in our ->map callback the lack of the > gpu hang checks doesn't matter. > > Furthermore the goal is to eventually have per dma-buf locking done > by callers with ww mutexes, so this will then be removed. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Ugh, who can't handle EINTR here but can handle all the other errors? -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map 2013-08-07 10:30 ` Chris Wilson @ 2013-08-07 10:49 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 10:49 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, DRI Development, Intel Graphics Development On Wed, Aug 7, 2013 at 12:30 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Aug 07, 2013 at 12:09:33PM +0200, Daniel Vetter wrote: >> It's unclear whether ->map is allowed to fail with -EINTR, but >> looking at current callers it's pretty clear that they don't >> expect this to happen. So use a blocking mutex_lock call. Since >> we don't wait for the gpu in our ->map callback the lack of the >> gpu hang checks doesn't matter. >> >> Furthermore the goal is to eventually have per dma-buf locking done >> by callers with ww mutexes, so this will then be removed. >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Ugh, who can't handle EINTR here but can handle all the other errors? Ok, I've re-read the code and I think callers can actually cope. I'm just freaked out that we don't have test coverage for these case, but that should be a moot point once all the locking is converted over to ww mutexes. So I'll drop this patch here. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 10:09 ` [PATCH 1/2] " Daniel Vetter 2013-08-07 10:09 ` [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map Daniel Vetter @ 2013-08-07 10:40 ` Maarten Lankhorst 2013-08-08 0:50 ` Konrad Rzeszutek Wilk 2 siblings, 0 replies; 30+ messages in thread From: Maarten Lankhorst @ 2013-08-07 10:40 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development Op 07-08-13 12:09, Daniel Vetter schreef: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > v2: Add locking to unmap, noticed by Chris Wilson. Note that even > though we call unmap with our own dev->struct_mutex held that won't > result in an immediate deadlock since we never go through the dma_buf > interfaces for our own, reimported buffers. But it's still easy to > blow up and anger lockdep, but that's already the case with our ->map > implementation. Fixing this for real will involve per dma-buf ww mutex > locking by the callers. And lots of fun. So go with the duct-tape > approach for now. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Tested-by: Armin K. <krejzi@email.com> (v1) > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Acked, this was my original patch to solve the issue. I want to note that locking struct_mutex here will break lockdep, but it's a problem in drm, not this patch. ~Maarten ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-07 10:09 ` [PATCH 1/2] " Daniel Vetter 2013-08-07 10:09 ` [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map Daniel Vetter 2013-08-07 10:40 ` [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap Maarten Lankhorst @ 2013-08-08 0:50 ` Konrad Rzeszutek Wilk 2013-08-08 1:20 ` Chris Wilson 2 siblings, 1 reply; 30+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-08-08 0:50 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > v2: Add locking to unmap, noticed by Chris Wilson. Note that even > though we call unmap with our own dev->struct_mutex held that won't > result in an immediate deadlock since we never go through the dma_buf > interfaces for our own, reimported buffers. But it's still easy to > blow up and anger lockdep, but that's already the case with our ->map > implementation. Fixing this for real will involve per dma-buf ww mutex > locking by the callers. And lots of fun. So go with the duct-tape > approach for now. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Tested-by: Armin K. <krejzi@email.com> (v1) > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index 63ee1a9..f7e1682 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, > struct sg_table *sg, > enum dma_data_direction dir) > { > + struct drm_i915_gem_object *obj = attachment->dmabuf->priv; > + > + mutex_lock(&obj->base.dev->struct_mutex); > + > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); > sg_free_table(sg); > kfree(sg); > + > + i915_gem_object_unpin_pages(obj); I am curious - would it logic of first unpinning, and then doing the dma_unmap_sg make more sense? As in, in the map path we do: dma_map pin And in here you do the same: dma_unmap unpin But I would have thought that on a unroll you would do it in reverse order, so: unpin dma_unmap > + > + mutex_unlock(&obj->base.dev->struct_mutex); > } > > static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > -- > 1.8.3.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap 2013-08-08 0:50 ` Konrad Rzeszutek Wilk @ 2013-08-08 1:20 ` Chris Wilson 0 siblings, 0 replies; 30+ messages in thread From: Chris Wilson @ 2013-08-08 1:20 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Daniel Vetter, Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 08:50:20PM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote: > > This fixes a WARN in i915_gem_free_object when the > > obj->pages_pin_count isn't 0. > > > > v2: Add locking to unmap, noticed by Chris Wilson. Note that even > > though we call unmap with our own dev->struct_mutex held that won't > > result in an immediate deadlock since we never go through the dma_buf > > interfaces for our own, reimported buffers. But it's still easy to > > blow up and anger lockdep, but that's already the case with our ->map > > implementation. Fixing this for real will involve per dma-buf ww mutex > > locking by the callers. And lots of fun. So go with the duct-tape > > approach for now. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > > Tested-by: Armin K. <krejzi@email.com> (v1) > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > index 63ee1a9..f7e1682 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, > > struct sg_table *sg, > > enum dma_data_direction dir) > > { > > + struct drm_i915_gem_object *obj = attachment->dmabuf->priv; > > + > > + mutex_lock(&obj->base.dev->struct_mutex); > > + > > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); > > sg_free_table(sg); > > kfree(sg); > > + > > + i915_gem_object_unpin_pages(obj); > > I am curious - would it logic of first unpinning, and then doing the dma_unmap_sg > make more sense? As in, in the map path we do: > > dma_map > pin Actually this is the wrong way around, and is a potential issue. Hindsight is a powerful tool. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv 2013-08-07 9:15 [PATCH 0/3] Small i915/exynos prime cleanup Daniel Vetter 2013-08-07 9:15 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter 2013-08-07 9:15 ` [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap Daniel Vetter @ 2013-08-07 9:15 ` Daniel Vetter 2013-08-07 9:43 ` Chris Wilson 2 siblings, 1 reply; 30+ messages in thread From: Daniel Vetter @ 2013-08-07 9:15 UTC (permalink / raw) To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development Makes it more obviously correct what tricks we play by reusing the drm prime release helper. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 0bf3d51..3c0edaf 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -27,10 +27,15 @@ #include "i915_drv.h" #include <linux/dma-buf.h> +static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) +{ + return to_intel_bo(buf->priv); +} + static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachment, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); struct sg_table *st; struct scatterlist *src, *dst; int ret, i; @@ -85,7 +90,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *sg, enum dma_data_direction dir) { - struct drm_i915_gem_object *obj = attachment->dmabuf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf); dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); @@ -96,7 +101,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) { - struct drm_i915_gem_object *obj = dma_buf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; struct sg_page_iter sg_iter; struct page **pages; @@ -144,7 +149,7 @@ error: static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) { - struct drm_i915_gem_object *obj = dma_buf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; int ret; @@ -187,7 +192,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct * static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size_t length, enum dma_data_direction direction) { - struct drm_i915_gem_object *obj = dma_buf->priv; + struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); struct drm_device *dev = obj->base.dev; int ret; bool write = (direction == DMA_BIDIRECTIONAL || direction == DMA_TO_DEVICE); @@ -218,9 +223,7 @@ static const struct dma_buf_ops i915_dmabuf_ops = { struct dma_buf *i915_gem_prime_export(struct drm_device *dev, struct drm_gem_object *gem_obj, int flags) { - struct drm_i915_gem_object *obj = to_intel_bo(gem_obj); - - return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, flags); + return dma_buf_export(gem_obj, &i915_dmabuf_ops, gem_obj->size, flags); } static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) @@ -257,7 +260,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, /* is this one of own objects? */ if (dma_buf->ops == &i915_dmabuf_ops) { - obj = dma_buf->priv; + obj = dma_buf_to_obj(dma_buf); /* is it from our device? */ if (obj->base.dev == dev) { /* -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv 2013-08-07 9:15 ` [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv Daniel Vetter @ 2013-08-07 9:43 ` Chris Wilson 0 siblings, 0 replies; 30+ messages in thread From: Chris Wilson @ 2013-08-07 9:43 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development On Wed, Aug 07, 2013 at 11:15:08AM +0200, Daniel Vetter wrote: > Makes it more obviously correct what tricks we play by reusing the drm > prime release helper. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> I do like these little typesafe cast helpers. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2013-08-08 6:58 UTC | newest] Thread overview: 30+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-07 9:15 [PATCH 0/3] Small i915/exynos prime cleanup Daniel Vetter 2013-08-07 9:15 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter 2013-08-07 9:37 ` [Intel-gfx] " Chris Wilson 2013-08-07 9:40 ` Inki Dae 2013-08-07 9:55 ` Daniel Vetter 2013-08-07 10:18 ` Joonyoung Shim 2013-08-07 10:21 ` Daniel Vetter 2013-08-07 10:29 ` Joonyoung Shim 2013-08-07 12:01 ` Inki Dae 2013-08-07 12:07 ` Daniel Vetter 2013-08-07 12:37 ` Inki Dae 2013-08-07 23:21 ` Daniel Vetter 2013-08-08 4:32 ` Inki Dae 2013-08-08 4:39 ` [PATCH 1/4] drm/exynos: explicit store base gem object in dma_buf->priv Inki Dae 2013-08-08 4:56 ` Inki Dae 2013-08-08 6:58 ` Daniel Vetter 2013-08-08 6:31 ` [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter 2013-08-07 9:15 ` [PATCH 2/3] drm/i915: unpin backing storage in dmabuf_unmap Daniel Vetter 2013-08-07 9:19 ` Daniel Vetter 2013-08-07 9:29 ` [Intel-gfx] " Chris Wilson 2013-08-07 9:49 ` Daniel Vetter 2013-08-07 10:09 ` [PATCH 1/2] " Daniel Vetter 2013-08-07 10:09 ` [PATCH 2/2] drm/i915: no interruptible locking for dma_buf->map Daniel Vetter 2013-08-07 10:30 ` Chris Wilson 2013-08-07 10:49 ` Daniel Vetter 2013-08-07 10:40 ` [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap Maarten Lankhorst 2013-08-08 0:50 ` Konrad Rzeszutek Wilk 2013-08-08 1:20 ` Chris Wilson 2013-08-07 9:15 ` [PATCH 3/3] drm/i915: explicit store base gem object in dma_buf->priv Daniel Vetter 2013-08-07 9:43 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.