Am 04.07.2017 um 17:56 schrieb Felix Kuehling:
> On 17-07-04 03:32 AM, Christian König wrote:
>> Am 03.07.2017 um 23:11 schrieb Felix Kuehling:
>>> +
>>> + list_add(&gobj->list, &bo->gem_objects);
>>> + gobj->bo = amdgpu_bo_ref(bo);
>>> + bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>> It's a bit more tricker than that. IIRC my original patch limited the
>> BO to GTT space as well.
> I dug out your original patch for reference (attached), which was
> originally applied on a 4.1-based KFD staging branch. Your original
> patch series also included a change for VRAM VM mappings with the system
> bit (also attached). So your original intention was clearly to also
> allow VRAM P2P access. The VM patch didn't apply after some VM changes
> (probably related to the vram manager) and was later replaced by Amber's
> patch.
Ok in this case my memory fooled me.
>> VRAM peer to peer access doesn't work with most PCIe chipsets.
>>
>> At bare minimum we need to put this behind a config option or add a
>> white list for the chipset or only enable it if
>> "pci=pcie_bus_peer2peer" is set or something like this.
> Well we're using it without any special pci= flags.
> pci=pcie_bus_peer2peer can reduce performance, so we should not require
> it if it's not needed on all systems.
>
> There are other issues that can prevent P2P access between some pairs of
> devices. For example on Intel dual-socket boards the QPI link between
> the sockets doesn't work for P2P traffic. So P2P only works between
> devices connected to the same socket.
>
> I think it's impractical to check all those chipset-specific limitations
> at this level.
Seriously? Sorry, but that approach is a clear no-go.
Long story short all those cases must cleanly be handled before this
patch series can land upstream (or even be in any hybrid release).
> Importing and mapping a foreign BO should be no problem
> either way. If remote access is limited, that's something the
> application can figure out on its own. In case of KFD, this is done
> based on IO-link topology information.
I'm pretty sure that the patch as is would break A+A laptops, so pushing
it to any branch outside some KFD testing environment is a clear NAK
from my side.
I would also strongly discourage to use it in a production system until
those issues are sorted out. This patch series was a technical prove of
concept, not something we can upstream as is.
What needs to be done is working on a white list for chipsets and/or
interconnections between PCIe bus systems.
Regards,
Christian.
>
> Regards,
> Felix
>
>
>> BTW: If you modify a patch as severely as that please add your
>> Signed-of-by line as well.
>>
>> Regards,
>> Christian.
>>
>>> +
>>> + ww_mutex_unlock(&bo->tbo.resv->lock);
>>> +
>>> + return &gobj->base;
>>> +}
>>> +
>>> +struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>>> + struct dma_buf *dma_buf)
>>> +{
>>> + struct amdgpu_device *adev = dev->dev_private;
>>> +
>>> + if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
>>> + struct drm_gem_object *obj = dma_buf->priv;
>>> +
>>> + if (obj->dev != dev && obj->dev->driver == dev->driver) {
>>> + /* It's a amdgpu_bo from a different driver instance */
>>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>> +
>>> + return amdgpu_gem_prime_foreign_bo(adev, bo);
>>> + }
>>> + }
>>> +
>>> + return drm_gem_prime_import(dev, dma_buf);
>>> +}
>>
