Re: [PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 30.04.19 um 16:16 schrieb Daniel Vetter:
> [SNIP]
>>   /**
>> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>> - * @dma_buf: Shared DMA buffer
>> + * amdgpu_gem_pin_dma_buf - &dma_buf_ops.pin_dma_buf implementation
>> + *
>> + * @dma_buf: DMA-buf to pin in memory
>> + *
>> + * Pin the BO which is backing the DMA-buf so that it can't move any more.
>> + */
>> +static int amdgpu_gem_pin_dma_buf(struct dma_buf *dma_buf)
>> +{
>> +	struct drm_gem_object *obj = dma_buf->priv;
>> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>> +
>> +	/* pin buffer into GTT */
>> +	return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
> This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
> this can fail is someone already pinned the buffer into vram. And that
> kind of checking is supposed to happen in the buffer attachment.

Why is that supposed to happen on the attachment? I mean it could be 
nice to have for debugging, but I still don't see any practical reason 
for this.

> Also will p2p pin into VRAM if all attachments are p2p capable? Or is your
> plan to require dynamic invalidate to avoid fragmenting vram badly with
> pinned stuff you can't move?

My plan was to make dynamic invalidation a must have for P2P, exactly 
for the reason you noted.

> +/**
> + * amdgpu_gem_dma_buf_attach - &dma_buf_ops.attach implementation
> + *
> + * @dma_buf: DMA-buf we attach to
>    * @attach: DMA-buf attachment
>    *
> + * Returns:
> + * Always zero for success.
> + */
> +static int amdgpu_gem_dma_buf_attach(struct dma_buf *dma_buf,
> +				     struct dma_buf_attachment *attach)
> +{
> +	struct drm_gem_object *obj = dma_buf->priv;
> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +
> +	/* Make sure the buffer is pinned when userspace didn't set GTT as
> +	 * preferred domain. This avoid ping/pong situations with scan out BOs.
> +	 */
> +	if (!(bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT))
> +		attach->invalidate = NULL;
> Not following here at all. If the BO can't be in GTT I'd guess you should
> reject the attach outright, since the pinning/map later on will fail I
> guess? At least I'm not making the connection with why dynamic dma-buf
> won't work anymore, since dynamic dma-buf is to make p2p of bo in vram
> work better, which is exactly what this here seems to check for.
>
> Or is this just a quick check until you add full p2p support?
>
> Count me confused ...

Well completely amdgpu internal handling here. Key point is we have both 
preferred_domains as well as allowed_domains.

During command submission we always try to move a BO to the 
preferred_domains again.

Now what could happen if we don't have this check is the following:

1. BO is allocate in VRAM. And preferred_domains says only VRAM please, 
but allowed_domains says VRAM or GTT.

2. DMA-buf Importer comes along and moves the BO to GTT, which is 
perfectly valid because of the allowed_domains.

3. Command submission is made and moves the BO to VRAM again.

4. Importer comes along and moves the BO to GTT.
....

E.g. a nice ping/pong situation which just eats up memory bandwidth.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel