Am 16.10.19 um 16:23 schrieb Daniel Vetter:
On Wed, Oct 16, 2019 at 3:46 PM Koenig, Christian Christian.Koenig@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).
Ok, in this case its my fault. I totally missed your reply on 1 and thought that the reply on 2 was actually for a different thread.
I'm going to submit the TTM changes separately, cause that is actually a bug fix for a completely different issue which just happens to surface because we change the locking.
Thanks, Christian.
-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