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 :(
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