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

[Daniel Vetter <daniel@ffwll.ch>]

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.
-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
http://blog.ffwll.ch