-----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