[Intel-gfx] [PATCH 1/3] drm: use common drm_gem_dmabuf_release in i915/exynos drivers

[Daniel Vetter]

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