[Intel-gfx] [PATCH 1/4] dma-buf: change DMA-buf locking convention

[Daniel Vetter]

On Wed, Oct 16, 2019 at 3:46 PM Koenig, Christian
<Christian.Koenig at amd.com> wrote:
>
> Am 08.10.19 um 10:55 schrieb Daniel Vetter:
> > On Wed, Oct 02, 2019 at 08:37:50AM +0000, Koenig, Christian wrote:
> >> Hi Daniel,
> >>
> >> once more a ping on this. Any more comments or can we get it comitted?
> > Sorry got a bit smashed past weeks, but should be resurrected now back
> > from xdc.
>
> And any more thoughts on this? I mean we are blocked for month on this
> now :(

I replied to both 1 and 2 in this series on 8th Oct. You even replied
to me in the thread on patch 2 ... but not here (I top posted since
this detour here just me being confused).
-Daniel

>
> Thanks,
> Christian.
>
> > -Daniel
> >> Thanks,
> >> Christian.
> >>
> >> Am 24.09.19 um 11:50 schrieb Christian König:
> >>> Am 17.09.19 um 16:56 schrieb Daniel Vetter:
> >>>> [SNIP]
> >>>>>>>>>>>>>>        +    /* When either the importer or the exporter
> >>>>>>>>>>>>>> can't handle dynamic
> >>>>>>>>>>>>>> +     * mappings we cache the mapping here to avoid issues
> >>>>>>>>>>>>>> with the
> >>>>>>>>>>>>>> +     * reservation object lock.
> >>>>>>>>>>>>>> +     */
> >>>>>>>>>>>>>> +    if (dma_buf_attachment_is_dynamic(attach) !=
> >>>>>>>>>>>>>> +        dma_buf_is_dynamic(dmabuf)) {
> >>>>>>>>>>>>>> +        struct sg_table *sgt;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +        if (dma_buf_is_dynamic(attach->dmabuf))
> >>>>>>>>>>>>>> + dma_resv_lock(attach->dmabuf->resv, NULL);
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +        sgt = dmabuf->ops->map_dma_buf(attach,
> >>>>>>>>>>>>>> DMA_BIDIRECTIONAL);
> >>>>>>>>>>>>> Now we're back to enforcing DMA_BIDI, which works nicely
> >>>>>>>>>>>>> around the
> >>>>>>>>>>>>> locking pain, but apparently upsets the arm-soc folks who
> >>>>>>>>>>>>> want to
> >>>>>>>>>>>>> control
> >>>>>>>>>>>>> this better.
> >>>>>>>>>>>> Take another look at dma_buf_map_attachment(), we still try
> >>>>>>>>>>>> to get the
> >>>>>>>>>>>> caching there for ARM.
> >>>>>>>>>>>>
> >>>>>>>>>>>> What we do here is to bidirectionally map the buffer to avoid
> >>>>>>>>>>>> the
> >>>>>>>>>>>> locking hydra when importer and exporter disagree on locking.
> >>>>>>>>>>>>
> >>>>>>>>>>>> So the ARM folks can easily avoid that by switching to
> >>>>>>>>>>>> dynamic locking
> >>>>>>>>>>>> for both.
> >>>>>>>>>> So you still break the contract between importer and exporter,
> >>>>>>>>>> except not
> >>>>>>>>>> for anything that's run in intel-gfx-ci so all is good?
> >>>>>>>>> No, the contract between importer and exporter stays exactly the
> >>>>>>>>> same it
> >>>>>>>>> is currently as long as you don't switch to dynamic dma-buf
> >>>>>>>>> handling.
> >>>>>>>>>
> >>>>>>>>> There is no functional change for the ARM folks here. The only
> >>>>>>>>> change
> >>>>>>>>> which takes effect is between i915 and amdgpu and that is perfectly
> >>>>>>>>> covered by intel-gfx-ci.
> >>>>>>>> There's people who want to run amdgpu on ARM?
> >>>>>>> Sure there are, we even recently fixed some bugs for this.
> >>>>>>>
> >>>>>>> But as far as I know there is no one currently which is affect by
> >>>>>>> this
> >>>>>>> change on ARM with amdgpu.
> >>>>>> But don't you break them with this now?
> >>>>> No, we see the bidirectional attachment as compatible with the other
> >>>>> ones.
> >>>>>
> >>>>>> amdgpu will soon set the dynamic flag on exports, which forces the
> >>>>>> caching
> >>>>>> at create time (to avoid the locking fun), which will then result in a
> >>>>>> EBUSY at map_attachment time because we have a cached mapping, but
> >>>>>> it's
> >>>>>> the wrong type.
> >>>>> See the check in dma_buf_map_attachment():
> >>>>>
> >>>>>        if (attach->dir != direction && attach->dir != DMA_BIDIRECTIONAL)
> >>>>>            return ERR_PTR(-EBUSY);
> >>>> Hm, I misread this. So yeah should work, +/- the issue that we might
> >>>> not flush enough. But I guess that can be fixed whenever, it's not
> >>>> like dma-api semantics are a great fit for us. Maybe a fixme comment
> >>>> would be useful here ... I'll look at this tomorrow or so because atm
> >>>> brain is slow, I'm down with the usual post-conference cold it seems
> >>>> :-/
> >>> Hope your are feeling better now, adding a comment is of course not a
> >>> problem.
> >>>
> >>> With that fixed can I get an reviewed-by or at least and acked-by?
> >>>
> >>> I want to land at least some parts of those changes now.
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>> -Daniel
> >>>>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch