First step towards unpinned DMA-buf operation.

First step towards unpinned DMA-buf operation.

[Christian König]

[As requested by Daniel cross posting to intel-gfx as well].

This set is the first step towards allowing to use a DMA-buf without actually pinning the underlying resources. This in turn the the ground work for PCIe P2P operations as well as quite a bunch of other use cases.

The idea is that we build the support for unpinned operation around the already present reservation lock in the DMA-buf object. For this we now grab the reservation object lock while mapping and unmapping DMA-bufs.

The down side is that all implementations as well as users of DMA-buf needs to be audited to make sure that we don't run into double locking or lock inversions.

So please test and/or comment and report back how badly lockdep complains :)

Thanks,
Christian.

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

[PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

[Christian König]

Add function variants which can be called with the reservation lock
already held.

v2: reordered, add lockdep asserts, fix kerneldoc

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-buf.h   |  5 +++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 852a3928ee71..dc94e76e2e2a 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 }
 EXPORT_SYMBOL_GPL(dma_buf_detach);
 
+/**
+ * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space
+ * with the reservation lock held. Is a wrapper for map_dma_buf() of the
+ *
+ * Returns the scatterlist table of the attachment;
+ * dma_buf_ops.
+ * @attach:	[in]	attachment whose scatterlist is to be returned
+ * @direction:	[in]	direction of DMA transfer
+ *
+ * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
+ * on error. May return -EINTR if it is interrupted by a signal.
+ *
+ * A mapping must be unmapped by using dma_buf_unmap_attachment_locked(). Note
+ * that the underlying backing storage is pinned for as long as a mapping
+ * exists, therefore users/importers should not hold onto a mapping for undue
+ * amounts of time.
+ */
+struct sg_table *
+dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
+			      enum dma_data_direction direction)
+{
+	struct sg_table *sg_table;
+
+	might_sleep();
+	reservation_object_assert_held(attach->dmabuf->resv);
+
+	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
+	if (!sg_table)
+		sg_table = ERR_PTR(-ENOMEM);
+
+	return sg_table;
+}
+EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
+
 /**
  * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
  * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
@@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 }
 EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
 
+/**
+ * dma_buf_unmap_attachment_locked - unmaps the buffer with reservation lock
+ * held, should deallocate the associated scatterlist. Is a wrapper for
+ * unmap_dma_buf() of dma_buf_ops.
+ * @attach:	[in]	attachment to unmap buffer from
+ * @sg_table:	[in]	scatterlist info of the buffer to unmap
+ * @direction:  [in]    direction of DMA transfer
+ *
+ * This unmaps a DMA mapping for @attached obtained by
+ * dma_buf_map_attachment_locked().
+ */
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
+				     struct sg_table *sg_table,
+				     enum dma_data_direction direction)
+{
+	might_sleep();
+	reservation_object_assert_held(attach->dmabuf->resv);
+
+	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
+						direction);
+}
+EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
+
 /**
  * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
  * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 991787a03199..a25e754ae2f7 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
 struct dma_buf *dma_buf_get(int fd);
 void dma_buf_put(struct dma_buf *dmabuf);
 
+struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,
+					       enum dma_data_direction);
 struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
 					enum dma_data_direction);
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
+				     struct sg_table *,
+				     enum dma_data_direction);
 void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
 				enum dma_data_direction);
 int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

[Daniel Vetter]

On Fri, Jun 22, 2018 at 04:11:00PM +0200, Christian König wrote:
> Add function variants which can be called with the reservation lock
> already held.
> 
> v2: reordered, add lockdep asserts, fix kerneldoc
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-buf.h   |  5 +++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 852a3928ee71..dc94e76e2e2a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_detach);
>  
> +/**
> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space
> + * with the reservation lock held. Is a wrapper for map_dma_buf() of the
> + *

This here looks mangled (and the blank line wreaks the resulting html).
With that fixed.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> + * Returns the scatterlist table of the attachment;
> + * dma_buf_ops.
> + * @attach:	[in]	attachment whose scatterlist is to be returned
> + * @direction:	[in]	direction of DMA transfer
> + *
> + * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
> + * on error. May return -EINTR if it is interrupted by a signal.
> + *
> + * A mapping must be unmapped by using dma_buf_unmap_attachment_locked(). Note
> + * that the underlying backing storage is pinned for as long as a mapping
> + * exists, therefore users/importers should not hold onto a mapping for undue
> + * amounts of time.
> + */
> +struct sg_table *
> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
> +			      enum dma_data_direction direction)
> +{
> +	struct sg_table *sg_table;
> +
> +	might_sleep();
> +	reservation_object_assert_held(attach->dmabuf->resv);
> +
> +	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	if (!sg_table)
> +		sg_table = ERR_PTR(-ENOMEM);
> +
> +	return sg_table;
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
> +
>  /**
>   * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
>   * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
> @@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>  
> +/**
> + * dma_buf_unmap_attachment_locked - unmaps the buffer with reservation lock
> + * held, should deallocate the associated scatterlist. Is a wrapper for
> + * unmap_dma_buf() of dma_buf_ops.
> + * @attach:	[in]	attachment to unmap buffer from
> + * @sg_table:	[in]	scatterlist info of the buffer to unmap
> + * @direction:  [in]    direction of DMA transfer
> + *
> + * This unmaps a DMA mapping for @attached obtained by
> + * dma_buf_map_attachment_locked().
> + */
> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
> +				     struct sg_table *sg_table,
> +				     enum dma_data_direction direction)
> +{
> +	might_sleep();
> +	reservation_object_assert_held(attach->dmabuf->resv);
> +
> +	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> +						direction);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
> +
>  /**
>   * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
>   * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 991787a03199..a25e754ae2f7 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>  struct dma_buf *dma_buf_get(int fd);
>  void dma_buf_put(struct dma_buf *dmabuf);
>  
> +struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,
> +					       enum dma_data_direction);
>  struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>  					enum dma_data_direction);
> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
> +				     struct sg_table *,
> +				     enum dma_data_direction);
>  void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
>  				enum dma_data_direction);
>  int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
> -- 
> 2.14.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

[Zhang, Jerry (Junwei)]

On 06/22/2018 10:11 PM, Christian König wrote:
> Add function variants which can be called with the reservation lock
> already held.
>
> v2: reordered, add lockdep asserts, fix kerneldoc
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/dma-buf/dma-buf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/dma-buf.h   |  5 +++++
>   2 files changed, 62 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 852a3928ee71..dc94e76e2e2a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>   }
>   EXPORT_SYMBOL_GPL(dma_buf_detach);
>
> +/**
> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space
> + * with the reservation lock held. Is a wrapper for map_dma_buf() of the
> + *
> + * Returns the scatterlist table of the attachment;
> + * dma_buf_ops.
> + * @attach:	[in]	attachment whose scatterlist is to be returned
> + * @direction:	[in]	direction of DMA transfer
> + *
> + * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
> + * on error. May return -EINTR if it is interrupted by a signal.
> + *
> + * A mapping must be unmapped by using dma_buf_unmap_attachment_locked(). Note
> + * that the underlying backing storage is pinned for as long as a mapping
> + * exists, therefore users/importers should not hold onto a mapping for undue
> + * amounts of time.
> + */
> +struct sg_table *
> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
> +			      enum dma_data_direction direction)
> +{
> +	struct sg_table *sg_table;
> +

Perhaps better to add some error check, like dma_buf_map_attachment()

WARN_ON(!attach || !attach->dmabuf)

Apart from that, it's
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

Jerry

> +	might_sleep();
> +	reservation_object_assert_held(attach->dmabuf->resv);
> +
> +	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	if (!sg_table)
> +		sg_table = ERR_PTR(-ENOMEM);
> +
> +	return sg_table;
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
> +
>   /**
>    * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
>    * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
> @@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>   }
>   EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>
> +/**
> + * dma_buf_unmap_attachment_locked - unmaps the buffer with reservation lock
> + * held, should deallocate the associated scatterlist. Is a wrapper for
> + * unmap_dma_buf() of dma_buf_ops.
> + * @attach:	[in]	attachment to unmap buffer from
> + * @sg_table:	[in]	scatterlist info of the buffer to unmap
> + * @direction:  [in]    direction of DMA transfer
> + *
> + * This unmaps a DMA mapping for @attached obtained by
> + * dma_buf_map_attachment_locked().
> + */
> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
> +				     struct sg_table *sg_table,
> +				     enum dma_data_direction direction)
> +{
> +	might_sleep();
> +	reservation_object_assert_held(attach->dmabuf->resv);
> +
> +	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> +						direction);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
> +
>   /**
>    * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
>    * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 991787a03199..a25e754ae2f7 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>   struct dma_buf *dma_buf_get(int fd);
>   void dma_buf_put(struct dma_buf *dmabuf);
>
> +struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,
> +					       enum dma_data_direction);
>   struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>   					enum dma_data_direction);
> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
> +				     struct sg_table *,
> +				     enum dma_data_direction);
>   void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
>   				enum dma_data_direction);
>   int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

[Zhang, Jerry (Junwei)]

On 06/22/2018 10:11 PM, Christian König wrote:
> Add function variants which can be called with the reservation lock
> already held.
>
> v2: reordered, add lockdep asserts, fix kerneldoc
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/dma-buf/dma-buf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/dma-buf.h   |  5 +++++
>   2 files changed, 62 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 852a3928ee71..dc94e76e2e2a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>   }
>   EXPORT_SYMBOL_GPL(dma_buf_detach);
>
> +/**
> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space
> + * with the reservation lock held. Is a wrapper for map_dma_buf() of the
> + *
> + * Returns the scatterlist table of the attachment;
> + * dma_buf_ops.
> + * @attach:	[in]	attachment whose scatterlist is to be returned
> + * @direction:	[in]	direction of DMA transfer
> + *
> + * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
> + * on error. May return -EINTR if it is interrupted by a signal.
> + *
> + * A mapping must be unmapped by using dma_buf_unmap_attachment_locked(). Note
> + * that the underlying backing storage is pinned for as long as a mapping
> + * exists, therefore users/importers should not hold onto a mapping for undue
> + * amounts of time.
> + */
> +struct sg_table *
> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
> +			      enum dma_data_direction direction)
> +{
> +	struct sg_table *sg_table;
> +

Perhaps better to add some error check, like dma_buf_map_attachment()

WARN_ON(!attach || !attach->dmabuf)

Apart from that, it's
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

Jerry

> +	might_sleep();
> +	reservation_object_assert_held(attach->dmabuf->resv);
> +
> +	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	if (!sg_table)
> +		sg_table = ERR_PTR(-ENOMEM);
> +
> +	return sg_table;
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
> +
>   /**
>    * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
>    * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
> @@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>   }
>   EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>
> +/**
> + * dma_buf_unmap_attachment_locked - unmaps the buffer with reservation lock
> + * held, should deallocate the associated scatterlist. Is a wrapper for
> + * unmap_dma_buf() of dma_buf_ops.
> + * @attach:	[in]	attachment to unmap buffer from
> + * @sg_table:	[in]	scatterlist info of the buffer to unmap
> + * @direction:  [in]    direction of DMA transfer
> + *
> + * This unmaps a DMA mapping for @attached obtained by
> + * dma_buf_map_attachment_locked().
> + */
> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
> +				     struct sg_table *sg_table,
> +				     enum dma_data_direction direction)
> +{
> +	might_sleep();
> +	reservation_object_assert_held(attach->dmabuf->resv);
> +
> +	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> +						direction);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
> +
>   /**
>    * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
>    * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 991787a03199..a25e754ae2f7 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>   struct dma_buf *dma_buf_get(int fd);
>   void dma_buf_put(struct dma_buf *dmabuf);
>
> +struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,
> +					       enum dma_data_direction);
>   struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>   					enum dma_data_direction);
> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
> +				     struct sg_table *,
> +				     enum dma_data_direction);
>   void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
>   				enum dma_data_direction);
>   int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

[Christian König]

Am 28.06.2018 um 11:53 schrieb Zhang, Jerry (Junwei):
> On 06/22/2018 10:11 PM, Christian König wrote:
>> Add function variants which can be called with the reservation lock
>> already held.
>>
>> v2: reordered, add lockdep asserts, fix kerneldoc
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-buf.c | 57 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/dma-buf.h   |  5 +++++
>>   2 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 852a3928ee71..dc94e76e2e2a 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, 
>> struct dma_buf_attachment *attach)
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_detach);
>>
>> +/**
>> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ 
>> address space
>> + * with the reservation lock held. Is a wrapper for map_dma_buf() of 
>> the
>> + *
>> + * Returns the scatterlist table of the attachment;
>> + * dma_buf_ops.
>> + * @attach:    [in]    attachment whose scatterlist is to be returned
>> + * @direction:    [in]    direction of DMA transfer
>> + *
>> + * Returns sg_table containing the scatterlist to be returned; 
>> returns ERR_PTR
>> + * on error. May return -EINTR if it is interrupted by a signal.
>> + *
>> + * A mapping must be unmapped by using 
>> dma_buf_unmap_attachment_locked(). Note
>> + * that the underlying backing storage is pinned for as long as a 
>> mapping
>> + * exists, therefore users/importers should not hold onto a mapping 
>> for undue
>> + * amounts of time.
>> + */
>> +struct sg_table *
>> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
>> +                  enum dma_data_direction direction)
>> +{
>> +    struct sg_table *sg_table;
>> +
>
> Perhaps better to add some error check, like dma_buf_map_attachment()
>
> WARN_ON(!attach || !attach->dmabuf)

Actually I wanted to remove those from the other functions as well.

WARN_ON and BUG_ON checks for NULL pointers before using them are 
totally pointless because they have the same effect as a crash.

Regards,
Christian.

>
> Apart from that, it's
> Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>
>
> Jerry
>
>> +    might_sleep();
>> +    reservation_object_assert_held(attach->dmabuf->resv);
>> +
>> +    sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>> +    if (!sg_table)
>> +        sg_table = ERR_PTR(-ENOMEM);
>> +
>> +    return sg_table;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
>> +
>>   /**
>>    * dma_buf_map_attachment - Returns the scatterlist table of the 
>> attachment;
>>    * mapped into _device_ address space. Is a wrapper for 
>> map_dma_buf() of the
>> @@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct 
>> dma_buf_attachment *attach,
>>   }
>>   EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>>
>> +/**
>> + * dma_buf_unmap_attachment_locked - unmaps the buffer with 
>> reservation lock
>> + * held, should deallocate the associated scatterlist. Is a wrapper for
>> + * unmap_dma_buf() of dma_buf_ops.
>> + * @attach:    [in]    attachment to unmap buffer from
>> + * @sg_table:    [in]    scatterlist info of the buffer to unmap
>> + * @direction:  [in]    direction of DMA transfer
>> + *
>> + * This unmaps a DMA mapping for @attached obtained by
>> + * dma_buf_map_attachment_locked().
>> + */
>> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
>> +                     struct sg_table *sg_table,
>> +                     enum dma_data_direction direction)
>> +{
>> +    might_sleep();
>> +    reservation_object_assert_held(attach->dmabuf->resv);
>> +
>> +    attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
>> +                        direction);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
>> +
>>   /**
>>    * dma_buf_unmap_attachment - unmaps and decreases usecount of the 
>> buffer;might
>>    * deallocate the scatterlist associated. Is a wrapper for 
>> unmap_dma_buf() of
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index 991787a03199..a25e754ae2f7 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>>   struct dma_buf *dma_buf_get(int fd);
>>   void dma_buf_put(struct dma_buf *dmabuf);
>>
>> +struct sg_table *dma_buf_map_attachment_locked(struct 
>> dma_buf_attachment *,
>> +                           enum dma_data_direction);
>>   struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>>                       enum dma_data_direction);
>> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
>> +                     struct sg_table *,
>> +                     enum dma_data_direction);
>>   void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct 
>> sg_table *,
>>                   enum dma_data_direction);
>>   int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>>

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

[PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Christian König]

First step towards unpinned DMA buf operation.

I've checked the DRM drivers to potential locking of the reservation
object, but essentially we need to audit all implementations of the
dma_buf _ops for this to work.

v2: reordered

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 9 ++++++---
 include/linux/dma-buf.h   | 4 ++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index dc94e76e2e2a..49f23b791eb8 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -665,7 +665,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 	if (WARN_ON(!attach || !attach->dmabuf))
 		return ERR_PTR(-EINVAL);
 
-	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
+	reservation_object_lock(attach->dmabuf->resv, NULL);
+	sg_table = dma_buf_map_attachment_locked(attach, direction);
+	reservation_object_unlock(attach->dmabuf->resv);
 	if (!sg_table)
 		sg_table = ERR_PTR(-ENOMEM);
 
@@ -715,8 +717,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
 		return;
 
-	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
-						direction);
+	reservation_object_lock(attach->dmabuf->resv, NULL);
+	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
+	reservation_object_unlock(attach->dmabuf->resv);
 }
 EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
 
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index a25e754ae2f7..024658d1f22e 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -118,6 +118,8 @@ struct dma_buf_ops {
 	 * any other kind of sharing that the exporter might wish to make
 	 * available to buffer-users.
 	 *
+	 * This is called with the dmabuf->resv object locked.
+	 *
 	 * Returns:
 	 *
 	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
@@ -138,6 +140,8 @@ struct dma_buf_ops {
 	 * It should also unpin the backing storage if this is the last mapping
 	 * of the DMA buffer, it the exporter supports backing storage
 	 * migration.
+	 *
+	 * This is called with the dmabuf->resv object locked.
 	 */
 	void (*unmap_dma_buf)(struct dma_buf_attachment *,
 			      struct sg_table *,
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Daniel Vetter]

On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
> First step towards unpinned DMA buf operation.
> 
> I've checked the DRM drivers to potential locking of the reservation
> object, but essentially we need to audit all implementations of the
> dma_buf _ops for this to work.
> 
> v2: reordered
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/dma-buf/dma-buf.c | 9 ++++++---
>  include/linux/dma-buf.h   | 4 ++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index dc94e76e2e2a..49f23b791eb8 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -665,7 +665,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  	if (WARN_ON(!attach || !attach->dmabuf))
>  		return ERR_PTR(-EINVAL);
>  
> -	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	reservation_object_lock(attach->dmabuf->resv, NULL);
> +	sg_table = dma_buf_map_attachment_locked(attach, direction);
> +	reservation_object_unlock(attach->dmabuf->resv);
>  	if (!sg_table)
>  		sg_table = ERR_PTR(-ENOMEM);
>  
> @@ -715,8 +717,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>  		return;
>  
> -	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> -						direction);
> +	reservation_object_lock(attach->dmabuf->resv, NULL);
> +	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
> +	reservation_object_unlock(attach->dmabuf->resv);
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>  
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index a25e754ae2f7..024658d1f22e 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -118,6 +118,8 @@ struct dma_buf_ops {
>  	 * any other kind of sharing that the exporter might wish to make
>  	 * available to buffer-users.
>  	 *
> +	 * This is called with the dmabuf->resv object locked.
> +	 *
>  	 * Returns:
>  	 *
>  	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
> @@ -138,6 +140,8 @@ struct dma_buf_ops {
>  	 * It should also unpin the backing storage if this is the last mapping
>  	 * of the DMA buffer, it the exporter supports backing storage
>  	 * migration.
> +	 *
> +	 * This is called with the dmabuf->resv object locked.
>  	 */
>  	void (*unmap_dma_buf)(struct dma_buf_attachment *,
>  			      struct sg_table *,
> -- 
> 2.14.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Daniel Vetter]

On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
> On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
> > First step towards unpinned DMA buf operation.
> > 
> > I've checked the DRM drivers to potential locking of the reservation
> > object, but essentially we need to audit all implementations of the
> > dma_buf _ops for this to work.
> > 
> > v2: reordered
> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok I did review drivers a bit, but apparently not well enough by far. i915
CI is unhappy:

https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html

So yeah inserting that lock in there isn't the most trivial thing :-/

I kinda assume that other drivers will have similar issues, e.g. omapdrm's
use of dev->struct_mutex also very much looks like it'll result in a new
locking inversion.
-Daniel

> 
> > ---
> >  drivers/dma-buf/dma-buf.c | 9 ++++++---
> >  include/linux/dma-buf.h   | 4 ++++
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index dc94e76e2e2a..49f23b791eb8 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -665,7 +665,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> >  	if (WARN_ON(!attach || !attach->dmabuf))
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> > +	reservation_object_lock(attach->dmabuf->resv, NULL);
> > +	sg_table = dma_buf_map_attachment_locked(attach, direction);
> > +	reservation_object_unlock(attach->dmabuf->resv);
> >  	if (!sg_table)
> >  		sg_table = ERR_PTR(-ENOMEM);
> >  
> > @@ -715,8 +717,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> >  	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> >  		return;
> >  
> > -	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> > -						direction);
> > +	reservation_object_lock(attach->dmabuf->resv, NULL);
> > +	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
> > +	reservation_object_unlock(attach->dmabuf->resv);
> >  }
> >  EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
> >  
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index a25e754ae2f7..024658d1f22e 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -118,6 +118,8 @@ struct dma_buf_ops {
> >  	 * any other kind of sharing that the exporter might wish to make
> >  	 * available to buffer-users.
> >  	 *
> > +	 * This is called with the dmabuf->resv object locked.
> > +	 *
> >  	 * Returns:
> >  	 *
> >  	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
> > @@ -138,6 +140,8 @@ struct dma_buf_ops {
> >  	 * It should also unpin the backing storage if this is the last mapping
> >  	 * of the DMA buffer, it the exporter supports backing storage
> >  	 * migration.
> > +	 *
> > +	 * This is called with the dmabuf->resv object locked.
> >  	 */
> >  	void (*unmap_dma_buf)(struct dma_buf_attachment *,
> >  			      struct sg_table *,
> > -- 
> > 2.14.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Christian König]

Am 25.06.2018 um 11:12 schrieb Daniel Vetter:
> On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
>> On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
>>> First step towards unpinned DMA buf operation.
>>>
>>> I've checked the DRM drivers to potential locking of the reservation
>>> object, but essentially we need to audit all implementations of the
>>> dma_buf _ops for this to work.
>>>
>>> v2: reordered
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Ok I did review drivers a bit, but apparently not well enough by far. i915
> CI is unhappy:
>
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
>
> So yeah inserting that lock in there isn't the most trivial thing :-/
>
> I kinda assume that other drivers will have similar issues, e.g. omapdrm's
> use of dev->struct_mutex also very much looks like it'll result in a new
> locking inversion.

Ah, crap. Already feared that this wouldn't be easy, but yeah that it is 
as bad as this is rather disappointing.

Thanks for the info, going to keep thinking about how to solve those issues.

Christian.

> -Daniel
>
>>> ---
>>>   drivers/dma-buf/dma-buf.c | 9 ++++++---
>>>   include/linux/dma-buf.h   | 4 ++++
>>>   2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index dc94e76e2e2a..49f23b791eb8 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -665,7 +665,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>>>   	if (WARN_ON(!attach || !attach->dmabuf))
>>>   		return ERR_PTR(-EINVAL);
>>>   
>>> -	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>>> +	reservation_object_lock(attach->dmabuf->resv, NULL);
>>> +	sg_table = dma_buf_map_attachment_locked(attach, direction);
>>> +	reservation_object_unlock(attach->dmabuf->resv);
>>>   	if (!sg_table)
>>>   		sg_table = ERR_PTR(-ENOMEM);
>>>   
>>> @@ -715,8 +717,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>>>   	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>>>   		return;
>>>   
>>> -	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
>>> -						direction);
>>> +	reservation_object_lock(attach->dmabuf->resv, NULL);
>>> +	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
>>> +	reservation_object_unlock(attach->dmabuf->resv);
>>>   }
>>>   EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>>>   
>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>>> index a25e754ae2f7..024658d1f22e 100644
>>> --- a/include/linux/dma-buf.h
>>> +++ b/include/linux/dma-buf.h
>>> @@ -118,6 +118,8 @@ struct dma_buf_ops {
>>>   	 * any other kind of sharing that the exporter might wish to make
>>>   	 * available to buffer-users.
>>>   	 *
>>> +	 * This is called with the dmabuf->resv object locked.
>>> +	 *
>>>   	 * Returns:
>>>   	 *
>>>   	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
>>> @@ -138,6 +140,8 @@ struct dma_buf_ops {
>>>   	 * It should also unpin the backing storage if this is the last mapping
>>>   	 * of the DMA buffer, it the exporter supports backing storage
>>>   	 * migration.
>>> +	 *
>>> +	 * This is called with the dmabuf->resv object locked.
>>>   	 */
>>>   	void (*unmap_dma_buf)(struct dma_buf_attachment *,
>>>   			      struct sg_table *,
>>> -- 
>>> 2.14.1
>>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Daniel Vetter]

On Tue, Jul 03, 2018 at 01:46:44PM +0200, Christian König wrote:
> Am 25.06.2018 um 11:12 schrieb Daniel Vetter:
> > On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
> > > On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
> > > > First step towards unpinned DMA buf operation.
> > > > 
> > > > I've checked the DRM drivers to potential locking of the reservation
> > > > object, but essentially we need to audit all implementations of the
> > > > dma_buf _ops for this to work.
> > > > 
> > > > v2: reordered
> > > > 
> > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Ok I did review drivers a bit, but apparently not well enough by far. i915
> > CI is unhappy:
> > 
> > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
> > 
> > So yeah inserting that lock in there isn't the most trivial thing :-/
> > 
> > I kinda assume that other drivers will have similar issues, e.g. omapdrm's
> > use of dev->struct_mutex also very much looks like it'll result in a new
> > locking inversion.
> 
> Ah, crap. Already feared that this wouldn't be easy, but yeah that it is as
> bad as this is rather disappointing.
> 
> Thanks for the info, going to keep thinking about how to solve those issues.

Side note: We want to make sure that drivers don't get the reservation_obj
locking hierarchy wrong in other places (using dev->struct_mutex is kinda
a pre-existing mis-use that we can't wish away retroactively
unfortunately). One really important thing is that shrinker vs resv_obj
must work with trylocks in the shrinker, so that you can allocate memory
while holding reservation objects.

One neat trick to teach lockdep about this would be to have a dummy

if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
	ww_mutex_lock(dma_buf->resv_obj);
	fs_reclaim_acquire(GFP_KERNEL);
	fs_reclaim_release(GFP_KERNEL);
	ww_mutex_unlock(dma_buf->resv_obj);
}

in dma_buf_init(). We're using the fs_reclaim_acquire/release check very
successfully to improve our igt test coverage for i915.ko in other areas.

Totally unrelated to dev->struct_mutex, but thoughts? Well for
dev->struct_mutex we could at least decide on one true way to nest
resv_obj vs. dev->struct_mutex as maybe an interim step, but not sure how
much that would help.
-Daniel

> 
> Christian.
> 
> > -Daniel
> > 
> > > > ---
> > > >   drivers/dma-buf/dma-buf.c | 9 ++++++---
> > > >   include/linux/dma-buf.h   | 4 ++++
> > > >   2 files changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > > index dc94e76e2e2a..49f23b791eb8 100644
> > > > --- a/drivers/dma-buf/dma-buf.c
> > > > +++ b/drivers/dma-buf/dma-buf.c
> > > > @@ -665,7 +665,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> > > >   	if (WARN_ON(!attach || !attach->dmabuf))
> > > >   		return ERR_PTR(-EINVAL);
> > > > -	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> > > > +	reservation_object_lock(attach->dmabuf->resv, NULL);
> > > > +	sg_table = dma_buf_map_attachment_locked(attach, direction);
> > > > +	reservation_object_unlock(attach->dmabuf->resv);
> > > >   	if (!sg_table)
> > > >   		sg_table = ERR_PTR(-ENOMEM);
> > > > @@ -715,8 +717,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> > > >   	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
> > > >   		return;
> > > > -	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> > > > -						direction);
> > > > +	reservation_object_lock(attach->dmabuf->resv, NULL);
> > > > +	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
> > > > +	reservation_object_unlock(attach->dmabuf->resv);
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
> > > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > > > index a25e754ae2f7..024658d1f22e 100644
> > > > --- a/include/linux/dma-buf.h
> > > > +++ b/include/linux/dma-buf.h
> > > > @@ -118,6 +118,8 @@ struct dma_buf_ops {
> > > >   	 * any other kind of sharing that the exporter might wish to make
> > > >   	 * available to buffer-users.
> > > >   	 *
> > > > +	 * This is called with the dmabuf->resv object locked.
> > > > +	 *
> > > >   	 * Returns:
> > > >   	 *
> > > >   	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
> > > > @@ -138,6 +140,8 @@ struct dma_buf_ops {
> > > >   	 * It should also unpin the backing storage if this is the last mapping
> > > >   	 * of the DMA buffer, it the exporter supports backing storage
> > > >   	 * migration.
> > > > +	 *
> > > > +	 * This is called with the dmabuf->resv object locked.
> > > >   	 */
> > > >   	void (*unmap_dma_buf)(struct dma_buf_attachment *,
> > > >   			      struct sg_table *,
> > > > -- 
> > > > 2.14.1
> > > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Christian König]

Am 03.07.2018 um 14:52 schrieb Daniel Vetter:
> On Tue, Jul 03, 2018 at 01:46:44PM +0200, Christian König wrote:
>> Am 25.06.2018 um 11:12 schrieb Daniel Vetter:
>>> On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
>>>> On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
>>>>> First step towards unpinned DMA buf operation.
>>>>>
>>>>> I've checked the DRM drivers to potential locking of the reservation
>>>>> object, but essentially we need to audit all implementations of the
>>>>> dma_buf _ops for this to work.
>>>>>
>>>>> v2: reordered
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Ok I did review drivers a bit, but apparently not well enough by far. i915
>>> CI is unhappy:
>>>
>>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
>>>
>>> So yeah inserting that lock in there isn't the most trivial thing :-/
>>>
>>> I kinda assume that other drivers will have similar issues, e.g. omapdrm's
>>> use of dev->struct_mutex also very much looks like it'll result in a new
>>> locking inversion.
>> Ah, crap. Already feared that this wouldn't be easy, but yeah that it is as
>> bad as this is rather disappointing.
>>
>> Thanks for the info, going to keep thinking about how to solve those issues.
> Side note: We want to make sure that drivers don't get the reservation_obj
> locking hierarchy wrong in other places (using dev->struct_mutex is kinda
> a pre-existing mis-use that we can't wish away retroactively
> unfortunately). One really important thing is that shrinker vs resv_obj
> must work with trylocks in the shrinker, so that you can allocate memory
> while holding reservation objects.
>
> One neat trick to teach lockdep about this would be to have a dummy
>
> if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
> 	ww_mutex_lock(dma_buf->resv_obj);
> 	fs_reclaim_acquire(GFP_KERNEL);
> 	fs_reclaim_release(GFP_KERNEL);
> 	ww_mutex_unlock(dma_buf->resv_obj);
> }
>
> in dma_buf_init(). We're using the fs_reclaim_acquire/release check very
> successfully to improve our igt test coverage for i915.ko in other areas.
>
> Totally unrelated to dev->struct_mutex, but thoughts? Well for
> dev->struct_mutex we could at least decide on one true way to nest
> resv_obj vs. dev->struct_mutex as maybe an interim step, but not sure how
> much that would help.

I don't think that would help. As far as I can see we only have two choices:

1. Either have a big patch which fixes all DMA-buf implementations to 
allow the reservation lock to be held during map/unmap (unrealistic).

2. Add a flag to at least in the mid term tell the DMA-buf helper 
functions what to do. E.g. create the mapping without the reservation 
lock held.


How about moving the SGL caching from the DRM layer into the DMA-buf 
layer and add a flag if the exporter wants/needs this caching?

Then only the implementations which can deal with dynamic invalidation 
disable SGL caching and with it enable creating the sgl with the 
reservation object locked.

This way we can kill two birds with one stone by both avoiding the SGL 
caching in the DRM layer as well as having a sane handling for the locking.

Thoughts?

Christian.


> -Daniel

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

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Daniel Vetter]

On Tue, Jul 03, 2018 at 03:02:11PM +0200, Christian König wrote:
> Am 03.07.2018 um 14:52 schrieb Daniel Vetter:
> > On Tue, Jul 03, 2018 at 01:46:44PM +0200, Christian König wrote:
> > > Am 25.06.2018 um 11:12 schrieb Daniel Vetter:
> > > > On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
> > > > > On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
> > > > > > First step towards unpinned DMA buf operation.
> > > > > > 
> > > > > > I've checked the DRM drivers to potential locking of the reservation
> > > > > > object, but essentially we need to audit all implementations of the
> > > > > > dma_buf _ops for this to work.
> > > > > > 
> > > > > > v2: reordered
> > > > > > 
> > > > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Ok I did review drivers a bit, but apparently not well enough by far. i915
> > > > CI is unhappy:
> > > > 
> > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
> > > > 
> > > > So yeah inserting that lock in there isn't the most trivial thing :-/
> > > > 
> > > > I kinda assume that other drivers will have similar issues, e.g. omapdrm's
> > > > use of dev->struct_mutex also very much looks like it'll result in a new
> > > > locking inversion.
> > > Ah, crap. Already feared that this wouldn't be easy, but yeah that it is as
> > > bad as this is rather disappointing.
> > > 
> > > Thanks for the info, going to keep thinking about how to solve those issues.
> > Side note: We want to make sure that drivers don't get the reservation_obj
> > locking hierarchy wrong in other places (using dev->struct_mutex is kinda
> > a pre-existing mis-use that we can't wish away retroactively
> > unfortunately). One really important thing is that shrinker vs resv_obj
> > must work with trylocks in the shrinker, so that you can allocate memory
> > while holding reservation objects.
> > 
> > One neat trick to teach lockdep about this would be to have a dummy
> > 
> > if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
> > 	ww_mutex_lock(dma_buf->resv_obj);
> > 	fs_reclaim_acquire(GFP_KERNEL);
> > 	fs_reclaim_release(GFP_KERNEL);
> > 	ww_mutex_unlock(dma_buf->resv_obj);
> > }
> > 
> > in dma_buf_init(). We're using the fs_reclaim_acquire/release check very
> > successfully to improve our igt test coverage for i915.ko in other areas.
> > 
> > Totally unrelated to dev->struct_mutex, but thoughts? Well for
> > dev->struct_mutex we could at least decide on one true way to nest
> > resv_obj vs. dev->struct_mutex as maybe an interim step, but not sure how
> > much that would help.
> 
> I don't think that would help. As far as I can see we only have two choices:
> 
> 1. Either have a big patch which fixes all DMA-buf implementations to allow
> the reservation lock to be held during map/unmap (unrealistic).
> 
> 2. Add a flag to at least in the mid term tell the DMA-buf helper functions
> what to do. E.g. create the mapping without the reservation lock held.
> 
> 
> How about moving the SGL caching from the DRM layer into the DMA-buf layer
> and add a flag if the exporter wants/needs this caching?
> 
> Then only the implementations which can deal with dynamic invalidation
> disable SGL caching and with it enable creating the sgl with the reservation
> object locked.
> 
> This way we can kill two birds with one stone by both avoiding the SGL
> caching in the DRM layer as well as having a sane handling for the locking.
> 
> Thoughts?

I don't see how the SGL stuff factors into neither the dev->struct_mutex
nor into the need to do allocations while holding resv_obj. Neither
changes by moving that piece around. At least as far as I can see it SGL
caching is fully orthogonal to any kind of locking fun.

Why do you see a connection here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Christian König]

Am 03.07.2018 um 15:11 schrieb Daniel Vetter:
> On Tue, Jul 03, 2018 at 03:02:11PM +0200, Christian König wrote:
>> Am 03.07.2018 um 14:52 schrieb Daniel Vetter:
>>> On Tue, Jul 03, 2018 at 01:46:44PM +0200, Christian König wrote:
>>>> Am 25.06.2018 um 11:12 schrieb Daniel Vetter:
>>>>> On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
>>>>>> On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
>>>>>>> First step towards unpinned DMA buf operation.
>>>>>>>
>>>>>>> I've checked the DRM drivers to potential locking of the reservation
>>>>>>> object, but essentially we need to audit all implementations of the
>>>>>>> dma_buf _ops for this to work.
>>>>>>>
>>>>>>> v2: reordered
>>>>>>>
>>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> Ok I did review drivers a bit, but apparently not well enough by far. i915
>>>>> CI is unhappy:
>>>>>
>>>>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
>>>>>
>>>>> So yeah inserting that lock in there isn't the most trivial thing :-/
>>>>>
>>>>> I kinda assume that other drivers will have similar issues, e.g. omapdrm's
>>>>> use of dev->struct_mutex also very much looks like it'll result in a new
>>>>> locking inversion.
>>>> Ah, crap. Already feared that this wouldn't be easy, but yeah that it is as
>>>> bad as this is rather disappointing.
>>>>
>>>> Thanks for the info, going to keep thinking about how to solve those issues.
>>> Side note: We want to make sure that drivers don't get the reservation_obj
>>> locking hierarchy wrong in other places (using dev->struct_mutex is kinda
>>> a pre-existing mis-use that we can't wish away retroactively
>>> unfortunately). One really important thing is that shrinker vs resv_obj
>>> must work with trylocks in the shrinker, so that you can allocate memory
>>> while holding reservation objects.
>>>
>>> One neat trick to teach lockdep about this would be to have a dummy
>>>
>>> if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
>>> 	ww_mutex_lock(dma_buf->resv_obj);
>>> 	fs_reclaim_acquire(GFP_KERNEL);
>>> 	fs_reclaim_release(GFP_KERNEL);
>>> 	ww_mutex_unlock(dma_buf->resv_obj);
>>> }
>>>
>>> in dma_buf_init(). We're using the fs_reclaim_acquire/release check very
>>> successfully to improve our igt test coverage for i915.ko in other areas.
>>>
>>> Totally unrelated to dev->struct_mutex, but thoughts? Well for
>>> dev->struct_mutex we could at least decide on one true way to nest
>>> resv_obj vs. dev->struct_mutex as maybe an interim step, but not sure how
>>> much that would help.
>> I don't think that would help. As far as I can see we only have two choices:
>>
>> 1. Either have a big patch which fixes all DMA-buf implementations to allow
>> the reservation lock to be held during map/unmap (unrealistic).
>>
>> 2. Add a flag to at least in the mid term tell the DMA-buf helper functions
>> what to do. E.g. create the mapping without the reservation lock held.
>>
>>
>> How about moving the SGL caching from the DRM layer into the DMA-buf layer
>> and add a flag if the exporter wants/needs this caching?
>>
>> Then only the implementations which can deal with dynamic invalidation
>> disable SGL caching and with it enable creating the sgl with the reservation
>> object locked.
>>
>> This way we can kill two birds with one stone by both avoiding the SGL
>> caching in the DRM layer as well as having a sane handling for the locking.
>>
>> Thoughts?
> I don't see how the SGL stuff factors into neither the dev->struct_mutex
> nor into the need to do allocations while holding resv_obj. Neither
> changes by moving that piece around. At least as far as I can see it SGL
> caching is fully orthogonal to any kind of locking fun.
>
> Why do you see a connection here?

When the exporter's map_dma_buf() callback can't be called with the 
reservation lock held we must call it before taking the lock.

Now importers able to deal with dynamic invalidation always want to call 
the callback with the reservation lock held. Otherwise they would have 
two different code paths, one for the dynamic invalidation and one for 
the pinning.

One possibility to solve that problem is to call map_dma_buf() during 
the attach phase and cache the resulting sg_table in the attach object.

Only when the exporter says: Ok I can deal with the reservation lock 
held during the map_dma_buf() callback we disable the caching.

The only possible drawback I can see is that we now always cache the 
sg_table in the DMA-buf handling, while previously we have done it in 
the DRM midlayer.

Christian.

> -Daniel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Zhang, Jerry (Junwei)]

On 06/22/2018 10:11 PM, Christian König wrote:
> First step towards unpinned DMA buf operation.
>
> I've checked the DRM drivers to potential locking of the reservation
> object, but essentially we need to audit all implementations of the
> dma_buf _ops for this to work.
>
> v2: reordered
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

looks good for me.

Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

Jerry
> ---
>   drivers/dma-buf/dma-buf.c | 9 ++++++---
>   include/linux/dma-buf.h   | 4 ++++
>   2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index dc94e76e2e2a..49f23b791eb8 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -665,7 +665,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>   	if (WARN_ON(!attach || !attach->dmabuf))
>   		return ERR_PTR(-EINVAL);
>
> -	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	reservation_object_lock(attach->dmabuf->resv, NULL);
> +	sg_table = dma_buf_map_attachment_locked(attach, direction);
> +	reservation_object_unlock(attach->dmabuf->resv);
>   	if (!sg_table)
>   		sg_table = ERR_PTR(-ENOMEM);
>
> @@ -715,8 +717,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>   	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>   		return;
>
> -	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> -						direction);
> +	reservation_object_lock(attach->dmabuf->resv, NULL);
> +	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
> +	reservation_object_unlock(attach->dmabuf->resv);
>   }
>   EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index a25e754ae2f7..024658d1f22e 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -118,6 +118,8 @@ struct dma_buf_ops {
>   	 * any other kind of sharing that the exporter might wish to make
>   	 * available to buffer-users.
>   	 *
> +	 * This is called with the dmabuf->resv object locked.
> +	 *
>   	 * Returns:
>   	 *
>   	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
> @@ -138,6 +140,8 @@ struct dma_buf_ops {
>   	 * It should also unpin the backing storage if this is the last mapping
>   	 * of the DMA buffer, it the exporter supports backing storage
>   	 * migration.
> +	 *
> +	 * This is called with the dmabuf->resv object locked.
>   	 */
>   	void (*unmap_dma_buf)(struct dma_buf_attachment *,
>   			      struct sg_table *,
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/amdgpu: add independent DMA-buf export v3

[Christian König]

The caching of SGT's done by the DRM code is actually quite harmful and
should probably removed altogether in the long term.

Start by providing a separate DMA-buf export implementation in amdgpu. This is
also a prerequisite of unpinned DMA-buf handling.

v2: fix unintended recursion, remove debugging leftovers
v3: split out from unpinned DMA-buf work

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 +++++++++++++------------------
 3 files changed, 39 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index d8e0cc08f9db..5e71af8dd3a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -373,7 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
 void amdgpu_gem_object_close(struct drm_gem_object *obj,
 				struct drm_file *file_priv);
 unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
-struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
 struct drm_gem_object *
 amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 				 struct dma_buf_attachment *attach,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index a549483032b0..cdf0be85d361 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -919,7 +919,6 @@ static struct drm_driver kms_driver = {
 	.gem_prime_export = amdgpu_gem_prime_export,
 	.gem_prime_import = amdgpu_gem_prime_import,
 	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
-	.gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,
 	.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
 	.gem_prime_vmap = amdgpu_gem_prime_vmap,
 	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index b2286bc41aec..038a8c8488b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -40,22 +40,6 @@
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
-/**
- * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table
- * implementation
- * @obj: GEM buffer object
- *
- * Returns:
- * A scatter/gather table for the pinned pages of the buffer object's memory.
- */
-struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)
-{
-	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
-	int npages = bo->tbo.num_pages;
-
-	return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
-}
-
 /**
  * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
  * @obj: GEM buffer object
@@ -189,35 +173,29 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 }
 
 /**
- * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
- * @dma_buf: shared DMA buffer
- * @target_dev: target device
+ * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation
  * @attach: DMA-buf attachment
+ * @dir: DMA direction
  *
  * Makes sure that the shared DMA buffer can be accessed by the target device.
  * For now, simply pins it to the GTT domain, where it should be accessible by
  * all DMA devices.
  *
  * Returns:
- * 0 on success or negative error code.
+ * sg_table filled with the DMA addresses to use or ERR_PRT with negative error
+ * code.
  */
-static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
-				 struct dma_buf_attachment *attach)
+static struct sg_table *
+amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
+		       enum dma_data_direction dir)
 {
+	struct dma_buf *dma_buf = attach->dmabuf;
 	struct drm_gem_object *obj = dma_buf->priv;
 	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+	struct sg_table *sgt;
 	long r;
 
-	r = drm_gem_map_attach(dma_buf, attach);
-	if (r)
-		return r;
-
-	r = amdgpu_bo_reserve(bo, false);
-	if (unlikely(r != 0))
-		goto error_detach;
-
-
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
 		 * Wait for all shared fences to complete before we switch to future
@@ -228,54 +206,62 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 							MAX_SCHEDULE_TIMEOUT);
 		if (unlikely(r < 0)) {
 			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
-			goto error_unreserve;
+			return ERR_PTR(r);
 		}
 	}
 
 	/* pin buffer into GTT */
 	r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);
 	if (r)
-		goto error_unreserve;
+		return ERR_PTR(r);
+
+	sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages);
+	if (IS_ERR(sgt))
+		return sgt;
+
+	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
+			      DMA_ATTR_SKIP_CPU_SYNC))
+		goto error_free;
 
 	if (attach->dev->driver != adev->dev->driver)
 		bo->prime_shared_count++;
 
-error_unreserve:
-	amdgpu_bo_unreserve(bo);
+	return sgt;
 
-error_detach:
-	if (r)
-		drm_gem_map_detach(dma_buf, attach);
-	return r;
+error_free:
+	sg_free_table(sgt);
+	kfree(sgt);
+	return ERR_PTR(-ENOMEM);
 }
 
 /**
- * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation
- * @dma_buf: shared DMA buffer
+ * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation
  * @attach: DMA-buf attachment
+ * @sgt: sg_table to unmap
+ * @dir: DMA direction
  *
  * This is called when a shared DMA buffer no longer needs to be accessible by
  * the other device. For now, simply unpins the buffer from GTT.
  */
-static void amdgpu_gem_map_detach(struct dma_buf *dma_buf,
-				  struct dma_buf_attachment *attach)
+static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+				     struct sg_table *sgt,
+				     enum dma_data_direction dir)
 {
+	struct dma_buf *dma_buf = attach->dmabuf;
 	struct drm_gem_object *obj = dma_buf->priv;
 	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
-	int ret = 0;
-
-	ret = amdgpu_bo_reserve(bo, true);
-	if (unlikely(ret != 0))
-		goto error;
 
 	amdgpu_bo_unpin(bo);
+
 	if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count)
 		bo->prime_shared_count--;
-	amdgpu_bo_unreserve(bo);
 
-error:
-	drm_gem_map_detach(dma_buf, attach);
+	if (sgt) {
+		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+		sg_free_table(sgt);
+		kfree(sgt);
+	}
 }
 
 /**
@@ -333,10 +319,8 @@ static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf,
 }
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops = {
-	.attach = amdgpu_gem_map_attach,
-	.detach = amdgpu_gem_map_detach,
-	.map_dma_buf = drm_gem_map_dma_buf,
-	.unmap_dma_buf = drm_gem_unmap_dma_buf,
+	.map_dma_buf = amdgpu_gem_map_dma_buf,
+	.unmap_dma_buf = amdgpu_gem_unmap_dma_buf,
 	.release = drm_gem_dmabuf_release,
 	.begin_cpu_access = amdgpu_gem_begin_cpu_access,
 	.map = drm_gem_dmabuf_kmap,
-- 
2.14.1

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

Re: [PATCH 3/4] drm/amdgpu: add independent DMA-buf export v3

[Zhang, Jerry (Junwei)]

On 06/22/2018 10:11 PM, Christian König wrote:
> The caching of SGT's done by the DRM code is actually quite harmful and
> should probably removed altogether in the long term.
>
> Start by providing a separate DMA-buf export implementation in amdgpu. This is
> also a prerequisite of unpinned DMA-buf handling.
>
> v2: fix unintended recursion, remove debugging leftovers
> v3: split out from unpinned DMA-buf work
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 +++++++++++++------------------
>   3 files changed, 39 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index d8e0cc08f9db..5e71af8dd3a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -373,7 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
>   void amdgpu_gem_object_close(struct drm_gem_object *obj,
>   				struct drm_file *file_priv);
>   unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
>   struct drm_gem_object *
>   amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>   				 struct dma_buf_attachment *attach,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index a549483032b0..cdf0be85d361 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -919,7 +919,6 @@ static struct drm_driver kms_driver = {
>   	.gem_prime_export = amdgpu_gem_prime_export,
>   	.gem_prime_import = amdgpu_gem_prime_import,
>   	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
> -	.gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,

I may miss some patches or important info.

Even applied the series of patch, I still could find below code in drm_gem_map_dma_buf().
In this case, it will cause NULL pointer access. Please help me out of here.
{{{
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
}}}

Jerry

>   	.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
>   	.gem_prime_vmap = amdgpu_gem_prime_vmap,
>   	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index b2286bc41aec..038a8c8488b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -40,22 +40,6 @@
>
>   static const struct dma_buf_ops amdgpu_dmabuf_ops;
>
> -/**
> - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table
> - * implementation
> - * @obj: GEM buffer object
> - *
> - * Returns:
> - * A scatter/gather table for the pinned pages of the buffer object's memory.
> - */
> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)
> -{
> -	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> -	int npages = bo->tbo.num_pages;
> -
> -	return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
> -}
> -
>   /**
>    * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
>    * @obj: GEM buffer object
> @@ -189,35 +173,29 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>   }
>
>   /**
> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> - * @dma_buf: shared DMA buffer
> - * @target_dev: target device
> + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation
>    * @attach: DMA-buf attachment
> + * @dir: DMA direction
>    *
>    * Makes sure that the shared DMA buffer can be accessed by the target device.
>    * For now, simply pins it to the GTT domain, where it should be accessible by
>    * all DMA devices.
>    *
>    * Returns:
> - * 0 on success or negative error code.
> + * sg_table filled with the DMA addresses to use or ERR_PRT with negative error
> + * code.
>    */
> -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
> -				 struct dma_buf_attachment *attach)
> +static struct sg_table *
> +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
> +		       enum dma_data_direction dir)
>   {
> +	struct dma_buf *dma_buf = attach->dmabuf;
>   	struct drm_gem_object *obj = dma_buf->priv;
>   	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +	struct sg_table *sgt;
>   	long r;
>
> -	r = drm_gem_map_attach(dma_buf, attach);
> -	if (r)
> -		return r;
> -
> -	r = amdgpu_bo_reserve(bo, false);
> -	if (unlikely(r != 0))
> -		goto error_detach;
> -
> -
>   	if (attach->dev->driver != adev->dev->driver) {
>   		/*
>   		 * Wait for all shared fences to complete before we switch to future
> @@ -228,54 +206,62 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>   							MAX_SCHEDULE_TIMEOUT);
>   		if (unlikely(r < 0)) {
>   			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> -			goto error_unreserve;
> +			return ERR_PTR(r);
>   		}
>   	}
>
>   	/* pin buffer into GTT */
>   	r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);
>   	if (r)
> -		goto error_unreserve;
> +		return ERR_PTR(r);
> +
> +	sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages);
> +	if (IS_ERR(sgt))
> +		return sgt;
> +
> +	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
> +			      DMA_ATTR_SKIP_CPU_SYNC))
> +		goto error_free;
>
>   	if (attach->dev->driver != adev->dev->driver)
>   		bo->prime_shared_count++;
>
> -error_unreserve:
> -	amdgpu_bo_unreserve(bo);
> +	return sgt;
>
> -error_detach:
> -	if (r)
> -		drm_gem_map_detach(dma_buf, attach);
> -	return r;
> +error_free:
> +	sg_free_table(sgt);
> +	kfree(sgt);
> +	return ERR_PTR(-ENOMEM);
>   }
>
>   /**
> - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation
> - * @dma_buf: shared DMA buffer
> + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation
>    * @attach: DMA-buf attachment
> + * @sgt: sg_table to unmap
> + * @dir: DMA direction
>    *
>    * This is called when a shared DMA buffer no longer needs to be accessible by
>    * the other device. For now, simply unpins the buffer from GTT.
>    */
> -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf,
> -				  struct dma_buf_attachment *attach)
> +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> +				     struct sg_table *sgt,
> +				     enum dma_data_direction dir)
>   {
> +	struct dma_buf *dma_buf = attach->dmabuf;
>   	struct drm_gem_object *obj = dma_buf->priv;
>   	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> -	int ret = 0;
> -
> -	ret = amdgpu_bo_reserve(bo, true);
> -	if (unlikely(ret != 0))
> -		goto error;
>
>   	amdgpu_bo_unpin(bo);
> +
>   	if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count)
>   		bo->prime_shared_count--;
> -	amdgpu_bo_unreserve(bo);
>
> -error:
> -	drm_gem_map_detach(dma_buf, attach);
> +	if (sgt) {
> +		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
> +		sg_free_table(sgt);
> +		kfree(sgt);
> +	}
>   }
>
>   /**
> @@ -333,10 +319,8 @@ static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf,
>   }
>
>   static const struct dma_buf_ops amdgpu_dmabuf_ops = {
> -	.attach = amdgpu_gem_map_attach,
> -	.detach = amdgpu_gem_map_detach,
> -	.map_dma_buf = drm_gem_map_dma_buf,
> -	.unmap_dma_buf = drm_gem_unmap_dma_buf,
> +	.map_dma_buf = amdgpu_gem_map_dma_buf,
> +	.unmap_dma_buf = amdgpu_gem_unmap_dma_buf,
>   	.release = drm_gem_dmabuf_release,
>   	.begin_cpu_access = amdgpu_gem_begin_cpu_access,
>   	.map = drm_gem_dmabuf_kmap,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/amdgpu: add independent DMA-buf export v3

[Christian König]

Am 28.06.2018 um 11:58 schrieb Zhang, Jerry (Junwei):
> On 06/22/2018 10:11 PM, Christian König wrote:
>> The caching of SGT's done by the DRM code is actually quite harmful and
>> should probably removed altogether in the long term.
>>
>> Start by providing a separate DMA-buf export implementation in 
>> amdgpu. This is
>> also a prerequisite of unpinned DMA-buf handling.
>>
>> v2: fix unintended recursion, remove debugging leftovers
>> v3: split out from unpinned DMA-buf work
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  1 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  1 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 
>> +++++++++++++------------------
>>   3 files changed, 39 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index d8e0cc08f9db..5e71af8dd3a7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -373,7 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object 
>> *obj,
>>   void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>                   struct drm_file *file_priv);
>>   unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
>> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object 
>> *obj);
>>   struct drm_gem_object *
>>   amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>>                    struct dma_buf_attachment *attach,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index a549483032b0..cdf0be85d361 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -919,7 +919,6 @@ static struct drm_driver kms_driver = {
>>       .gem_prime_export = amdgpu_gem_prime_export,
>>       .gem_prime_import = amdgpu_gem_prime_import,
>>       .gem_prime_res_obj = amdgpu_gem_prime_res_obj,
>> -    .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,
>
> I may miss some patches or important info.
>
> Even applied the series of patch, I still could find below code in 
> drm_gem_map_dma_buf().
> In this case, it will cause NULL pointer access. Please help me out of 
> here.
> {{{
> sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> }}}

Take a look at the change a bit further down.

>
> Jerry
>
>>       .gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
>>       .gem_prime_vmap = amdgpu_gem_prime_vmap,
>>       .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>> index b2286bc41aec..038a8c8488b7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>> @@ -40,22 +40,6 @@
>>
>>   static const struct dma_buf_ops amdgpu_dmabuf_ops;
>>
>> -/**
>> - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table
>> - * implementation
>> - * @obj: GEM buffer object
>> - *
>> - * Returns:
>> - * A scatter/gather table for the pinned pages of the buffer 
>> object's memory.
>> - */
>> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object 
>> *obj)
>> -{
>> -    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>> -    int npages = bo->tbo.num_pages;
>> -
>> -    return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
>> -}
>> -
>>   /**
>>    * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
>>    * @obj: GEM buffer object
>> @@ -189,35 +173,29 @@ amdgpu_gem_prime_import_sg_table(struct 
>> drm_device *dev,
>>   }
>>
>>   /**
>> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>> - * @dma_buf: shared DMA buffer
>> - * @target_dev: target device
>> + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation
>>    * @attach: DMA-buf attachment
>> + * @dir: DMA direction
>>    *
>>    * Makes sure that the shared DMA buffer can be accessed by the 
>> target device.
>>    * For now, simply pins it to the GTT domain, where it should be 
>> accessible by
>>    * all DMA devices.
>>    *
>>    * Returns:
>> - * 0 on success or negative error code.
>> + * sg_table filled with the DMA addresses to use or ERR_PRT with 
>> negative error
>> + * code.
>>    */
>> -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>> -                 struct dma_buf_attachment *attach)
>> +static struct sg_table *
>> +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
>> +               enum dma_data_direction dir)
>>   {
>> +    struct dma_buf *dma_buf = attach->dmabuf;
>>       struct drm_gem_object *obj = dma_buf->priv;
>>       struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> +    struct sg_table *sgt;
>>       long r;
>>
>> -    r = drm_gem_map_attach(dma_buf, attach);
>> -    if (r)
>> -        return r;
>> -
>> -    r = amdgpu_bo_reserve(bo, false);
>> -    if (unlikely(r != 0))
>> -        goto error_detach;
>> -
>> -
>>       if (attach->dev->driver != adev->dev->driver) {
>>           /*
>>            * Wait for all shared fences to complete before we switch 
>> to future
>> @@ -228,54 +206,62 @@ static int amdgpu_gem_map_attach(struct dma_buf 
>> *dma_buf,
>>                               MAX_SCHEDULE_TIMEOUT);
>>           if (unlikely(r < 0)) {
>>               DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
>> -            goto error_unreserve;
>> +            return ERR_PTR(r);
>>           }
>>       }
>>
>>       /* pin buffer into GTT */
>>       r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL);
>>       if (r)
>> -        goto error_unreserve;
>> +        return ERR_PTR(r);
>> +
>> +    sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages);
>> +    if (IS_ERR(sgt))
>> +        return sgt;
>> +
>> +    if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
>> +                  DMA_ATTR_SKIP_CPU_SYNC))
>> +        goto error_free;
>>
>>       if (attach->dev->driver != adev->dev->driver)
>>           bo->prime_shared_count++;
>>
>> -error_unreserve:
>> -    amdgpu_bo_unreserve(bo);
>> +    return sgt;
>>
>> -error_detach:
>> -    if (r)
>> -        drm_gem_map_detach(dma_buf, attach);
>> -    return r;
>> +error_free:
>> +    sg_free_table(sgt);
>> +    kfree(sgt);
>> +    return ERR_PTR(-ENOMEM);
>>   }
>>
>>   /**
>> - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation
>> - * @dma_buf: shared DMA buffer
>> + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation
>>    * @attach: DMA-buf attachment
>> + * @sgt: sg_table to unmap
>> + * @dir: DMA direction
>>    *
>>    * This is called when a shared DMA buffer no longer needs to be 
>> accessible by
>>    * the other device. For now, simply unpins the buffer from GTT.
>>    */
>> -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf,
>> -                  struct dma_buf_attachment *attach)
>> +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>> +                     struct sg_table *sgt,
>> +                     enum dma_data_direction dir)
>>   {
>> +    struct dma_buf *dma_buf = attach->dmabuf;
>>       struct drm_gem_object *obj = dma_buf->priv;
>>       struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> -    int ret = 0;
>> -
>> -    ret = amdgpu_bo_reserve(bo, true);
>> -    if (unlikely(ret != 0))
>> -        goto error;
>>
>>       amdgpu_bo_unpin(bo);
>> +
>>       if (attach->dev->driver != adev->dev->driver && 
>> bo->prime_shared_count)
>>           bo->prime_shared_count--;
>> -    amdgpu_bo_unreserve(bo);
>>
>> -error:
>> -    drm_gem_map_detach(dma_buf, attach);
>> +    if (sgt) {
>> +        dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> +        sg_free_table(sgt);
>> +        kfree(sgt);
>> +    }
>>   }
>>
>>   /**
>> @@ -333,10 +319,8 @@ static int amdgpu_gem_begin_cpu_access(struct 
>> dma_buf *dma_buf,
>>   }
>>
>>   static const struct dma_buf_ops amdgpu_dmabuf_ops = {
>> -    .attach = amdgpu_gem_map_attach,
>> -    .detach = amdgpu_gem_map_detach,
>> -    .map_dma_buf = drm_gem_map_dma_buf,
>> -    .unmap_dma_buf = drm_gem_unmap_dma_buf,

Here we are stopping to use drm_gem_map_dma_buf(), so we don't need 
amdgpu_gem_prime_get_sg_table() any more either.

Christian.

>> +    .map_dma_buf = amdgpu_gem_map_dma_buf,
>> +    .unmap_dma_buf = amdgpu_gem_unmap_dma_buf,
>>       .release = drm_gem_dmabuf_release,
>>       .begin_cpu_access = amdgpu_gem_begin_cpu_access,
>>       .map = drm_gem_dmabuf_kmap,
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/amdgpu: add independent DMA-buf import v4

[Christian König]

Instead of relying on the DRM functions just implement our own import
functions. This prepares support for taking care of unpinned DMA-buf.

v2: enable for all exporters, not just amdgpu, fix invalidation
    handling, lock reservation object while setting callback
v3: change to new dma_buf attach interface
v4: split out from unpinned DMA-buf work

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  4 ----
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 38 +++++++++++++++++++------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   | 34 +++++++++++++++++++++++----
 4 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 5e71af8dd3a7..391c171f814e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -373,10 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
 void amdgpu_gem_object_close(struct drm_gem_object *obj,
 				struct drm_file *file_priv);
 unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
-struct drm_gem_object *
-amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
-				 struct dma_buf_attachment *attach,
-				 struct sg_table *sg);
 struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 					struct drm_gem_object *gobj,
 					int flags);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index cdf0be85d361..ad0a8b3f90b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -919,7 +919,6 @@ static struct drm_driver kms_driver = {
 	.gem_prime_export = amdgpu_gem_prime_export,
 	.gem_prime_import = amdgpu_gem_prime_import,
 	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
-	.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
 	.gem_prime_vmap = amdgpu_gem_prime_vmap,
 	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
 	.gem_prime_mmap = amdgpu_gem_prime_mmap,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 038a8c8488b7..5cc4c09d720e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -122,31 +122,28 @@ int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma
 }
 
 /**
- * amdgpu_gem_prime_import_sg_table - &drm_driver.gem_prime_import_sg_table
- * implementation
+ * amdgpu_gem_prime_create_obj - create BO for DMA-buf import
+ *
  * @dev: DRM device
- * @attach: DMA-buf attachment
- * @sg: Scatter/gather table
+ * @dma_buf: DMA-buf
  *
- * Import shared DMA buffer memory exported by another device.
+ * Creates an empty SG BO for DMA-buf import.
  *
  * Returns:
  * A new GEM buffer object of the given DRM device, representing the memory
  * described by the given DMA-buf attachment and scatter/gather table.
  */
-struct drm_gem_object *
-amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
-				 struct dma_buf_attachment *attach,
-				 struct sg_table *sg)
+static struct drm_gem_object *
+amdgpu_gem_prime_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)
 {
-	struct reservation_object *resv = attach->dmabuf->resv;
+	struct reservation_object *resv = dma_buf->resv;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_bo *bo;
 	struct amdgpu_bo_param bp;
 	int ret;
 
 	memset(&bp, 0, sizeof(bp));
-	bp.size = attach->dmabuf->size;
+	bp.size = dma_buf->size;
 	bp.byte_align = PAGE_SIZE;
 	bp.domain = AMDGPU_GEM_DOMAIN_CPU;
 	bp.flags = 0;
@@ -157,11 +154,9 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	if (ret)
 		goto error;
 
-	bo->tbo.sg = sg;
-	bo->tbo.ttm->sg = sg;
 	bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
 	bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
-	if (attach->dmabuf->ops != &amdgpu_dmabuf_ops)
+	if (dma_buf->ops != &amdgpu_dmabuf_ops)
 		bo->prime_shared_count = 1;
 
 	ww_mutex_unlock(&resv->lock);
@@ -376,6 +371,7 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 					    struct dma_buf *dma_buf)
 {
+	struct dma_buf_attachment *attach;
 	struct drm_gem_object *obj;
 
 	if (dma_buf->ops == &amdgpu_dmabuf_ops) {
@@ -390,5 +386,17 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 		}
 	}
 
-	return drm_gem_prime_import(dev, dma_buf);
+	obj = amdgpu_gem_prime_create_obj(dev, dma_buf);
+	if (IS_ERR(obj))
+		return obj;
+
+	attach = dma_buf_attach(dma_buf, dev->dev);
+	if (IS_ERR(attach)) {
+		drm_gem_object_put(obj);
+		return ERR_CAST(attach);
+	}
+
+	get_dma_buf(dma_buf);
+	obj->import_attach = attach;
+	return obj;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 0c084d3d0865..f2903054db9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
 #include <linux/pagemap.h>
 #include <linux/debugfs.h>
 #include <linux/iommu.h>
+#include <linux/dma-buf.h>
 #include "amdgpu.h"
 #include "amdgpu_object.h"
 #include "amdgpu_trace.h"
@@ -798,6 +799,7 @@ struct amdgpu_ttm_gup_task_list {
 
 struct amdgpu_ttm_tt {
 	struct ttm_dma_tt	ttm;
+	struct drm_gem_object	*gobj;
 	u64			offset;
 	uint64_t		userptr;
 	struct task_struct	*usertask;
@@ -1222,6 +1224,7 @@ static struct ttm_tt *amdgpu_ttm_tt_create(struct ttm_buffer_object *bo,
 		return NULL;
 	}
 	gtt->ttm.ttm.func = &amdgpu_backend_func;
+	gtt->gobj = &ttm_to_amdgpu_bo(bo)->gem_base;
 
 	/* allocate space for the uninitialized page entries */
 	if (ttm_sg_tt_init(&gtt->ttm, bo, page_flags)) {
@@ -1242,7 +1245,6 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(ttm->bdev);
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
-	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
 
 	/* user pages are bound by amdgpu_ttm_tt_pin_userptr() */
 	if (gtt && gtt->userptr) {
@@ -1255,7 +1257,20 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
 		return 0;
 	}
 
-	if (slave && ttm->sg) {
+	if (ttm->page_flags & TTM_PAGE_FLAG_SG) {
+		if (!ttm->sg) {
+			struct dma_buf_attachment *attach;
+			struct sg_table *sgt;
+
+			attach = gtt->gobj->import_attach;
+			sgt = dma_buf_map_attachment_locked(attach,
+							    DMA_BIDIRECTIONAL);
+			if (IS_ERR(sgt))
+				return PTR_ERR(sgt);
+
+			ttm->sg = sgt;
+		}
+
 		drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
 						 gtt->ttm.dma_address,
 						 ttm->num_pages);
@@ -1282,9 +1297,8 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
  */
 static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
 {
-	struct amdgpu_device *adev;
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
-	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
+	struct amdgpu_device *adev;
 
 	if (gtt && gtt->userptr) {
 		amdgpu_ttm_tt_set_user_pages(ttm, NULL);
@@ -1293,7 +1307,17 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
 		return;
 	}
 
-	if (slave)
+	if (ttm->sg && gtt->gobj->import_attach) {
+		struct dma_buf_attachment *attach;
+
+		attach = gtt->gobj->import_attach;
+		dma_buf_unmap_attachment_locked(attach, ttm->sg,
+						DMA_BIDIRECTIONAL);
+		ttm->sg = NULL;
+		return;
+	}
+
+	if (ttm->page_flags & TTM_PAGE_FLAG_SG)
 		return;
 
 	adev = amdgpu_ttm_adev(ttm->bdev);
-- 
2.14.1

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