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

[Daniel Vetter <daniel@ffwll.ch>]

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