Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-08 Thread Daniel Vetter 
On Thu, Aug 8, 2013 at 6:32 AM, Inki Dae inki@samsung.com wrote:


 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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 dan...@ffwll.ch
 
   On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae inki@samsung.com wrote:
   
   
2013/8/7 Daniel Vetter dan...@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@samsung.com
   wrote:
 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Inki Dae 


 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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@samsung.com
 Cc: Intel Graphics Development intel-...@lists.freedesktop.org
 Signed-off-by: Daniel Vetter daniel.vet...@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
 +++

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Daniel Vetter 
On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae inki@samsung.com wrote:
 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Joonyoung Shim 

On 08/07/2013 06:55 PM, Daniel Vetter wrote:

On Wed, Aug 7, 2013 at 11:40 AM, Inki Dae inki@samsung.com wrote:

-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@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


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Daniel Vetter 
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@samsung.com wrote:
 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Joonyoung Shim 

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@samsung.com wrote:

-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Inki Dae 
2013/8/7 Daniel Vetter dan...@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@samsung.com wrote:
  -Original Message-
  From: Daniel Vetter [mailto:daniel.vet...@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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Daniel Vetter 
On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae inki@samsung.com wrote:


 2013/8/7 Daniel Vetter dan...@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@samsung.com wrote:
  -Original Message-
  From: Daniel Vetter [mailto:daniel.vet...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Inki Dae 
2013/8/7 Daniel Vetter dan...@ffwll.ch

 On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae inki@samsung.com wrote:
 
 
  2013/8/7 Daniel Vetter dan...@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@samsung.com
 wrote:
   -Original Message-
   From: Daniel Vetter [mailto:daniel.vet...@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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Daniel Vetter 
On Wed, Aug 07, 2013 at 09:37:52PM +0900, Inki Dae wrote:
 2013/8/7 Daniel Vetter dan...@ffwll.ch
 
  On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae inki@samsung.com wrote:
  
  
   2013/8/7 Daniel Vetter dan...@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@samsung.com
  wrote:
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

2013-08-07 Thread Inki Dae 


 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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 dan...@ffwll.ch
 
   On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae inki@samsung.com wrote:
   
   
2013/8/7 Daniel Vetter dan...@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@samsung.com
   wrote:
 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel