On Thu, Aug 8, 2013 at 6:32 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: > > >> -----Original Message----- >> From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] 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@xxxxxxxx> >> > >> > > On Wed, Aug 7, 2013 at 2:01 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >> > > > >> > > > >> > > > 2013/8/7 Daniel Vetter <daniel@xxxxxxxx> >> > > >> >> > > >> 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@xxxxxxxxxxx> >> > > wrote: >> > > >> > >>>-----Original Message----- >> > > >> > >>>From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] >> > > >> > >>>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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
