[RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Seung-Woo Kim]

importer private data in dma-buf attachment can be used by importer to
reimport same dma-buf.

Seung-Woo Kim (2):
  dma-buf: add importer private data to attachment
  drm/prime: find gem object from the reimported dma-buf

 drivers/base/dma-buf.c                     |   31 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_prime.c                |   19 ++++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |    1 +
 drivers/gpu/drm/i915/i915_gem_dmabuf.c     |    1 +
 drivers/gpu/drm/udl/udl_gem.c              |    1 +
 include/linux/dma-buf.h                    |    4 +++
 6 files changed, 52 insertions(+), 5 deletions(-)

-- 
1.7.4.1

[RFC][PATCH 1/2] dma-buf: add importer private data to attachment

[Seung-Woo Kim]

dma-buf attachment has only exporter private data, but importer private data
can be useful for importer especially to re-import the same dma-buf.
To use importer private data in attachment of the device, the function to
search attachment in the attachment list of dma-buf is also added.

Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
 drivers/base/dma-buf.c  |   31 +++++++++++++++++++++++++++++++
 include/linux/dma-buf.h |    4 ++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index 08fe897..a1eaaf2 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -259,6 +259,37 @@ err_attach:
 EXPORT_SYMBOL_GPL(dma_buf_attach);
 
 /**
+ * dma_buf_get_attachment - Get attachment with the device from dma_buf's
+ * attachments list
+ * @dmabuf:	[in]	buffer to find device from.
+ * @dev:	[in]	device to be found.
+ *
+ * Returns struct dma_buf_attachment * attaching the device; may return
+ * negative error codes.
+ *
+ */
+struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf,
+						  struct device *dev)
+{
+	struct dma_buf_attachment *attach;
+
+	if (WARN_ON(!dmabuf || !dev))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&dmabuf->lock);
+	list_for_each_entry(attach, &dmabuf->attachments, node) {
+		if (attach->dev == dev) {
+			mutex_unlock(&dmabuf->lock);
+			return attach;
+		}
+	}
+	mutex_unlock(&dmabuf->lock);
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL_GPL(dma_buf_get_attachment);
+
+/**
  * dma_buf_detach - Remove the given attachment from dmabuf's attachments list;
  * optionally calls detach() of dma_buf_ops for device-specific detach
  * @dmabuf:	[in]	buffer to detach from.
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index dfac5ed..09cff0f 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -136,6 +136,7 @@ struct dma_buf {
  * @dev: device attached to the buffer.
  * @node: list of dma_buf_attachment.
  * @priv: exporter specific attachment data.
+ * @importer_priv: importer specific attachment data.
  *
  * This structure holds the attachment information between the dma_buf buffer
  * and its user device(s). The list contains one attachment struct per device
@@ -146,6 +147,7 @@ struct dma_buf_attachment {
 	struct device *dev;
 	struct list_head node;
 	void *priv;
+	void *importer_priv;
 };
 
 /**
@@ -164,6 +166,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
 
 struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 							struct device *dev);
+struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf,
+							struct device *dev);
 void dma_buf_detach(struct dma_buf *dmabuf,
 				struct dma_buf_attachment *dmabuf_attach);
 
-- 
1.7.4.1

Re: [RFC][PATCH 1/2] dma-buf: add importer private data to attachment

[Maarten Lankhorst]

Op 31-05-13 10:54, Seung-Woo Kim schreef:
> dma-buf attachment has only exporter private data, but importer private data
> can be useful for importer especially to re-import the same dma-buf.
> To use importer private data in attachment of the device, the function to
> search attachment in the attachment list of dma-buf is also added.
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
>  drivers/base/dma-buf.c  |   31 +++++++++++++++++++++++++++++++
>  include/linux/dma-buf.h |    4 ++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index 08fe897..a1eaaf2 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -259,6 +259,37 @@ err_attach:
>  EXPORT_SYMBOL_GPL(dma_buf_attach);
>  
>  /**
> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's
> + * attachments list
> + * @dmabuf:	[in]	buffer to find device from.
> + * @dev:	[in]	device to be found.
> + *
> + * Returns struct dma_buf_attachment * attaching the device; may return
> + * negative error codes.
> + *
> + */
> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf,
> +						  struct device *dev)
> +{
> +	struct dma_buf_attachment *attach;
> +
> +	if (WARN_ON(!dmabuf || !dev))
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&dmabuf->lock);
> +	list_for_each_entry(attach, &dmabuf->attachments, node) {
> +		if (attach->dev == dev) {
> +			mutex_unlock(&dmabuf->lock);
> +			return attach;
> +		}
> +	}
> +	mutex_unlock(&dmabuf->lock);
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment);
NAK in any form..

Spot the race condition between dma_buf_get_attachment and dma_buf_attach....

~Maarten

Re: [RFC][PATCH 1/2] dma-buf: add importer private data to attachment

[김승우]

Hello Maarten,

On 2013년 06월 05일 22:23, Maarten Lankhorst wrote:
> Op 31-05-13 10:54, Seung-Woo Kim schreef:
>> dma-buf attachment has only exporter private data, but importer private data
>> can be useful for importer especially to re-import the same dma-buf.
>> To use importer private data in attachment of the device, the function to
>> search attachment in the attachment list of dma-buf is also added.
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>> ---
>>  drivers/base/dma-buf.c  |   31 +++++++++++++++++++++++++++++++
>>  include/linux/dma-buf.h |    4 ++++
>>  2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> index 08fe897..a1eaaf2 100644
>> --- a/drivers/base/dma-buf.c
>> +++ b/drivers/base/dma-buf.c
>> @@ -259,6 +259,37 @@ err_attach:
>>  EXPORT_SYMBOL_GPL(dma_buf_attach);
>>  
>>  /**
>> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's
>> + * attachments list
>> + * @dmabuf:	[in]	buffer to find device from.
>> + * @dev:	[in]	device to be found.
>> + *
>> + * Returns struct dma_buf_attachment * attaching the device; may return
>> + * negative error codes.
>> + *
>> + */
>> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf,
>> +						  struct device *dev)
>> +{
>> +	struct dma_buf_attachment *attach;
>> +
>> +	if (WARN_ON(!dmabuf || !dev))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	mutex_lock(&dmabuf->lock);
>> +	list_for_each_entry(attach, &dmabuf->attachments, node) {
>> +		if (attach->dev == dev) {
>> +			mutex_unlock(&dmabuf->lock);
>> +			return attach;
>> +		}
>> +	}
>> +	mutex_unlock(&dmabuf->lock);
>> +
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment);
> NAK in any form..
> 
> Spot the race condition between dma_buf_get_attachment and dma_buf_attach....

Both dma_buf_get_attachment and dma_buf_attach has mutet with
dmabuf->lock, and dma_buf_get_attachment is used for get attachment from
same device before calling dma_buf_attach.

While, dma_buf_detach can removes attachment because it does not have
ref count. So importer should check ref count in its importer private
data before calling dma_buf_detach if the importer want to use
dma_buf_get_attachment.

And dma_buf_get_attachment is for the importer, so exporter attach and
detach callback operation should not call it as like exporter detach
callback operation should not call dma_buf_attach if you mean this kind
of race.

If you have other considerations here, please describe more specifically.

Thanks and Best Regards,
- Seung-Woo Kim

> 
> ~Maarten
> 
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: [RFC][PATCH 1/2] dma-buf: add importer private data to attachment

[Maarten Lankhorst]

Op 07-06-13 04:32, 김승우 schreef:
> Hello Maarten,
>
> On 2013년 06월 05일 22:23, Maarten Lankhorst wrote:
>> Op 31-05-13 10:54, Seung-Woo Kim schreef:
>>> dma-buf attachment has only exporter private data, but importer private data
>>> can be useful for importer especially to re-import the same dma-buf.
>>> To use importer private data in attachment of the device, the function to
>>> search attachment in the attachment list of dma-buf is also added.
>>>
>>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>>> ---
>>>  drivers/base/dma-buf.c  |   31 +++++++++++++++++++++++++++++++
>>>  include/linux/dma-buf.h |    4 ++++
>>>  2 files changed, 35 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>>> index 08fe897..a1eaaf2 100644
>>> --- a/drivers/base/dma-buf.c
>>> +++ b/drivers/base/dma-buf.c
>>> @@ -259,6 +259,37 @@ err_attach:
>>>  EXPORT_SYMBOL_GPL(dma_buf_attach);
>>>  
>>>  /**
>>> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's
>>> + * attachments list
>>> + * @dmabuf:	[in]	buffer to find device from.
>>> + * @dev:	[in]	device to be found.
>>> + *
>>> + * Returns struct dma_buf_attachment * attaching the device; may return
>>> + * negative error codes.
>>> + *
>>> + */
>>> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf,
>>> +						  struct device *dev)
>>> +{
>>> +	struct dma_buf_attachment *attach;
>>> +
>>> +	if (WARN_ON(!dmabuf || !dev))
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	mutex_lock(&dmabuf->lock);
>>> +	list_for_each_entry(attach, &dmabuf->attachments, node) {
>>> +		if (attach->dev == dev) {
>>> +			mutex_unlock(&dmabuf->lock);
>>> +			return attach;
>>> +		}
>>> +	}
>>> +	mutex_unlock(&dmabuf->lock);
>>> +
>>> +	return ERR_PTR(-ENODEV);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment);
>> NAK in any form..
>>
>> Spot the race condition between dma_buf_get_attachment and dma_buf_attach....
> Both dma_buf_get_attachment and dma_buf_attach has mutet with
> dmabuf->lock, and dma_buf_get_attachment is used for get attachment from
> same device before calling dma_buf_attach.

hint: what happens if 2 threads do this:

if (IS_ERR(attach = dma_buf_get_attachment(buf, dev)))
attach = dma_buf_attach(buf, dev);

There really is no correct usecase for this kind of thing, so please don't do it.

> While, dma_buf_detach can removes attachment because it does not have
> ref count. So importer should check ref count in its importer private
> data before calling dma_buf_detach if the importer want to use
> dma_buf_get_attachment.
>
> And dma_buf_get_attachment is for the importer, so exporter attach and
> detach callback operation should not call it as like exporter detach
> callback operation should not call dma_buf_attach if you mean this kind
> of race.
>
> If you have other considerations here, please describe more specifically.
>
> Thanks and Best Regards,
> - Seung-Woo Kim
>
>> ~Maarten
>>
>>

Re: [RFC][PATCH 1/2] dma-buf: add importer private data to attachment

[김승우]

On 2013년 06월 07일 20:24, Maarten Lankhorst wrote:
> Op 07-06-13 04:32, 김승우 schreef:
>> Hello Maarten,
>>
>> On 2013년 06월 05일 22:23, Maarten Lankhorst wrote:
>>> Op 31-05-13 10:54, Seung-Woo Kim schreef:
>>>> dma-buf attachment has only exporter private data, but importer private data
>>>> can be useful for importer especially to re-import the same dma-buf.
>>>> To use importer private data in attachment of the device, the function to
>>>> search attachment in the attachment list of dma-buf is also added.
>>>>
>>>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>>>> ---
>>>>  drivers/base/dma-buf.c  |   31 +++++++++++++++++++++++++++++++
>>>>  include/linux/dma-buf.h |    4 ++++
>>>>  2 files changed, 35 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>>>> index 08fe897..a1eaaf2 100644
>>>> --- a/drivers/base/dma-buf.c
>>>> +++ b/drivers/base/dma-buf.c
>>>> @@ -259,6 +259,37 @@ err_attach:
>>>>  EXPORT_SYMBOL_GPL(dma_buf_attach);
>>>>  
>>>>  /**
>>>> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's
>>>> + * attachments list
>>>> + * @dmabuf:	[in]	buffer to find device from.
>>>> + * @dev:	[in]	device to be found.
>>>> + *
>>>> + * Returns struct dma_buf_attachment * attaching the device; may return
>>>> + * negative error codes.
>>>> + *
>>>> + */
>>>> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf,
>>>> +						  struct device *dev)
>>>> +{
>>>> +	struct dma_buf_attachment *attach;
>>>> +
>>>> +	if (WARN_ON(!dmabuf || !dev))
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	mutex_lock(&dmabuf->lock);
>>>> +	list_for_each_entry(attach, &dmabuf->attachments, node) {
>>>> +		if (attach->dev == dev) {
>>>> +			mutex_unlock(&dmabuf->lock);
>>>> +			return attach;
>>>> +		}
>>>> +	}
>>>> +	mutex_unlock(&dmabuf->lock);
>>>> +
>>>> +	return ERR_PTR(-ENODEV);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment);
>>> NAK in any form..
>>>
>>> Spot the race condition between dma_buf_get_attachment and dma_buf_attach....
>> Both dma_buf_get_attachment and dma_buf_attach has mutet with
>> dmabuf->lock, and dma_buf_get_attachment is used for get attachment from
>> same device before calling dma_buf_attach.
> 
> hint: what happens if 2 threads do this:
> 
> if (IS_ERR(attach = dma_buf_get_attachment(buf, dev)))
> attach = dma_buf_attach(buf, dev);
> 
> There really is no correct usecase for this kind of thing, so please don't do it.

Ok, dma_buf_get_attachment can not prevent attachments from one device.

Anyway, do you think that importer side private data, not to allow
re-import one dma-buf to a device, has some advantage?
If that, I'll add some check_and_attach function, otherwise, I'll find
other way.

Thanks and Regards,
- Seung-Woo Kim

> 
>> While, dma_buf_detach can removes attachment because it does not have
>> ref count. So importer should check ref count in its importer private
>> data before calling dma_buf_detach if the importer want to use
>> dma_buf_get_attachment.
>>
>> And dma_buf_get_attachment is for the importer, so exporter attach and
>> detach callback operation should not call it as like exporter detach
>> callback operation should not call dma_buf_attach if you mean this kind
>> of race.
>>
>> If you have other considerations here, please describe more specifically.
>>
>> Thanks and Best Regards,
>> - Seung-Woo Kim
>>
>>> ~Maarten
>>>
>>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

[RFC][PATCH 2/2] drm/prime: find gem object from the reimported dma-buf

[Seung-Woo Kim]

Reimported dma-buf can reuse same gem object only when prime import
is done with same drm open context. So prime import is done with
other drm open context, gem object is newly created and mapped even
there is already mapped gem object. To avoid recreating gem object,
importer private data can be used at reimport time if it is assigned
with drm gem object at first import.
This can also remove remapping dma address for the hardware having
its own iommu.

Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
 drivers/gpu/drm/drm_prime.c                |   19 ++++++++++++++-----
 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |    1 +
 drivers/gpu/drm/i915/i915_gem_dmabuf.c     |    1 +
 drivers/gpu/drm/udl/udl_gem.c              |    1 +
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index dcde352..78a3c7d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -294,6 +294,7 @@ struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 	}
 
 	obj->import_attach = attach;
+	attach->importer_priv = obj;
 
 	return obj;
 
@@ -312,6 +313,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 {
 	struct dma_buf *dma_buf;
 	struct drm_gem_object *obj;
+	struct dma_buf_attachment *attach;
 	int ret;
 
 	dma_buf = dma_buf_get(prime_fd);
@@ -327,11 +329,18 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 		goto out_put;
 	}
 
-	/* never seen this one, need to import */
-	obj = dev->driver->gem_prime_import(dev, dma_buf);
-	if (IS_ERR(obj)) {
-		ret = PTR_ERR(obj);
-		goto out_put;
+	attach = dma_buf_get_attachment(dma_buf, dev->dev);
+	if (IS_ERR(attach)) {
+		/* never seen this one, need to import */
+		obj = dev->driver->gem_prime_import(dev, dma_buf);
+		if (IS_ERR(obj)) {
+			ret = PTR_ERR(obj);
+			goto out_put;
+		}
+	} else {
+		/* found attachment to same device */
+		obj = attach->importer_priv;
+		drm_gem_object_reference(obj);
 	}
 
 	ret = drm_gem_handle_create(file_priv, obj, handle);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index ff7f2a8..268da36 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -285,6 +285,7 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
 	exynos_gem_obj->buffer = buffer;
 	buffer->sgt = sgt;
 	exynos_gem_obj->base.import_attach = attach;
+	attach->importer_priv = &exynos_gem_obj->base;
 
 	DRM_DEBUG_PRIME("dma_addr = 0x%x, size = 0x%lx\n", buffer->dma_addr,
 								buffer->size);
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index dc53a52..75ef28c 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -297,6 +297,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 
 	i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops);
 	obj->base.import_attach = attach;
+	attach->importer_priv = &obj->base;
 
 	return &obj->base;
 
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index ef034fa..0652db1 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -317,6 +317,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
 	}
 
 	uobj->base.import_attach = attach;
+	attach->importer_priv = &uobj->base;
 
 	return &uobj->base;
 
-- 
1.7.4.1

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Daniel Vetter]

On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> importer private data in dma-buf attachment can be used by importer to
> reimport same dma-buf.
>
> Seung-Woo Kim (2):
>   dma-buf: add importer private data to attachment
>   drm/prime: find gem object from the reimported dma-buf

Self-import should already work (at least with the latest refcount
fixes merged). At least the tests to check both re-import on the same
drm fd and on a different all work as expected now.

Second, the dma_buf_attachment is _definitely_ the wrong place to do
this. If you need iommu mapping caching, that should happen at a lower
level (i.e. in the map_attachment callback somewhere of the exporter,
that's what the priv field in the attachment is for). Snatching away
the attachement from some random other import is certainly not the way
to go - attachements are _not_ refcounted!
-Daniel

>
>  drivers/base/dma-buf.c                     |   31 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_prime.c                |   19 ++++++++++++----
>  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |    1 +
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c     |    1 +
>  drivers/gpu/drm/udl/udl_gem.c              |    1 +
>  include/linux/dma-buf.h                    |    4 +++
>  6 files changed, 52 insertions(+), 5 deletions(-)
>
> --
> 1.7.4.1
>



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[김승우]

Hello Daniel,

Thanks for your comment.

On 2013년 05월 31일 18:14, Daniel Vetter wrote:
> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>> importer private data in dma-buf attachment can be used by importer to
>> reimport same dma-buf.
>>
>> Seung-Woo Kim (2):
>>   dma-buf: add importer private data to attachment
>>   drm/prime: find gem object from the reimported dma-buf
> 
> Self-import should already work (at least with the latest refcount
> fixes merged). At least the tests to check both re-import on the same
> drm fd and on a different all work as expected now.

Currently, prime works well for all case including self-importing,
importing, and reimporting as you describe. Just, importing dma-buf from
other driver twice with different drm_fd, each import create its own gem
object even two import is done for same buffer because prime_priv is in
struct drm_file. This means mapping to the device is done also twice.
IMHO, these duplicated creations and maps are not necessary if drm can
find previous import in different prime_priv.

> 
> Second, the dma_buf_attachment is _definitely_ the wrong place to do
> this. If you need iommu mapping caching, that should happen at a lower
> level (i.e. in the map_attachment callback somewhere of the exporter,
> that's what the priv field in the attachment is for). Snatching away
> the attachement from some random other import is certainly not the way
> to go - attachements are _not_ refcounted!

Yes, attachments do not have refcount, so importer should handle and drm
case in my patch, importer private data is gem object and it has, of
course, refcount.

And at current, exporter can not classify map_dma_buf requests of same
importer to same buffer with different attachment because dma_buf_attach
always makes new attachments. To resolve this exporter should search all
different attachment from same importer of dma-buf and it seems more
complex than importer private data to me.

If I misunderstood something, please let me know.

Best Regards,
- Seung-Woo Kim

> -Daniel
> 
>>
>>  drivers/base/dma-buf.c                     |   31 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_prime.c                |   19 ++++++++++++----
>>  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |    1 +
>>  drivers/gpu/drm/i915/i915_gem_dmabuf.c     |    1 +
>>  drivers/gpu/drm/udl/udl_gem.c              |    1 +
>>  include/linux/dma-buf.h                    |    4 +++
>>  6 files changed, 52 insertions(+), 5 deletions(-)
>>
>> --
>> 1.7.4.1
>>
> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Daniel Vetter]

On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
> Hello Daniel,
> 
> Thanks for your comment.
> 
> On 2013년 05월 31일 18:14, Daniel Vetter wrote:
> > On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> >> importer private data in dma-buf attachment can be used by importer to
> >> reimport same dma-buf.
> >>
> >> Seung-Woo Kim (2):
> >>   dma-buf: add importer private data to attachment
> >>   drm/prime: find gem object from the reimported dma-buf
> > 
> > Self-import should already work (at least with the latest refcount
> > fixes merged). At least the tests to check both re-import on the same
> > drm fd and on a different all work as expected now.
> 
> Currently, prime works well for all case including self-importing,
> importing, and reimporting as you describe. Just, importing dma-buf from
> other driver twice with different drm_fd, each import create its own gem
> object even two import is done for same buffer because prime_priv is in
> struct drm_file. This means mapping to the device is done also twice.
> IMHO, these duplicated creations and maps are not necessary if drm can
> find previous import in different prime_priv.

Well, that's imo a bug with the other driver. If it doesn't export
something really simple (e.g. contiguous memory which doesn't require any
mmio resources at all) it should have a cache of exported dma_buf fds so
that it hands out the same dma_buf every time.

Or it needs to be more clever in it's dma_buf_attachment_map functions and
lookup up a pre-existing iommu mapping.

But dealing with this in the importer is just broken.

> > Second, the dma_buf_attachment is _definitely_ the wrong place to do
> > this. If you need iommu mapping caching, that should happen at a lower
> > level (i.e. in the map_attachment callback somewhere of the exporter,
> > that's what the priv field in the attachment is for). Snatching away
> > the attachement from some random other import is certainly not the way
> > to go - attachements are _not_ refcounted!
> 
> Yes, attachments do not have refcount, so importer should handle and drm
> case in my patch, importer private data is gem object and it has, of
> course, refcount.
> 
> And at current, exporter can not classify map_dma_buf requests of same
> importer to same buffer with different attachment because dma_buf_attach
> always makes new attachments. To resolve this exporter should search all
> different attachment from same importer of dma-buf and it seems more
> complex than importer private data to me.
> 
> If I misunderstood something, please let me know.

Like I've said above, just fix this in the exporter. If an importer sees
two different dma_bufs it can very well presume that it those two indeed
point to different backing storage.

This will be even more important if we attach fences two dma_bufs. If your
broken exporter creates multiple dma_bufs each one of them will have their
own fences attached, leading to a complete disasters. Ok, strictly
speaking if you keep the same reservation pointer for each dma_buf it'll
work, but that's just a detail of how you solve this in the exporter.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Lucas Stach]

Am Freitag, den 31.05.2013, 17:29 +0200 schrieb Daniel Vetter:
> On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
> > Hello Daniel,
> > 
> > Thanks for your comment.
> > 
> > On 2013년 05월 31일 18:14, Daniel Vetter wrote:
> > > On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> > >> importer private data in dma-buf attachment can be used by importer to
> > >> reimport same dma-buf.
> > >>
> > >> Seung-Woo Kim (2):
> > >>   dma-buf: add importer private data to attachment
> > >>   drm/prime: find gem object from the reimported dma-buf
> > > 
> > > Self-import should already work (at least with the latest refcount
> > > fixes merged). At least the tests to check both re-import on the same
> > > drm fd and on a different all work as expected now.
> > 
> > Currently, prime works well for all case including self-importing,
> > importing, and reimporting as you describe. Just, importing dma-buf from
> > other driver twice with different drm_fd, each import create its own gem
> > object even two import is done for same buffer because prime_priv is in
> > struct drm_file. This means mapping to the device is done also twice.
> > IMHO, these duplicated creations and maps are not necessary if drm can
> > find previous import in different prime_priv.
> 
> Well, that's imo a bug with the other driver. If it doesn't export
> something really simple (e.g. contiguous memory which doesn't require any
> mmio resources at all) it should have a cache of exported dma_buf fds so
> that it hands out the same dma_buf every time.

I agree with the reasoning here.

Though it would be nice to have this "expected driver behavior" put down
somewhere in the documentation. Any volunteers?

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Daniel Vetter]

On Fri, May 31, 2013 at 06:21:09PM +0200, Lucas Stach wrote:
> Am Freitag, den 31.05.2013, 17:29 +0200 schrieb Daniel Vetter:
> > On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
> > > Hello Daniel,
> > > 
> > > Thanks for your comment.
> > > 
> > > On 2013년 05월 31일 18:14, Daniel Vetter wrote:
> > > > On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> > > >> importer private data in dma-buf attachment can be used by importer to
> > > >> reimport same dma-buf.
> > > >>
> > > >> Seung-Woo Kim (2):
> > > >>   dma-buf: add importer private data to attachment
> > > >>   drm/prime: find gem object from the reimported dma-buf
> > > > 
> > > > Self-import should already work (at least with the latest refcount
> > > > fixes merged). At least the tests to check both re-import on the same
> > > > drm fd and on a different all work as expected now.
> > > 
> > > Currently, prime works well for all case including self-importing,
> > > importing, and reimporting as you describe. Just, importing dma-buf from
> > > other driver twice with different drm_fd, each import create its own gem
> > > object even two import is done for same buffer because prime_priv is in
> > > struct drm_file. This means mapping to the device is done also twice.
> > > IMHO, these duplicated creations and maps are not necessary if drm can
> > > find previous import in different prime_priv.
> > 
> > Well, that's imo a bug with the other driver. If it doesn't export
> > something really simple (e.g. contiguous memory which doesn't require any
> > mmio resources at all) it should have a cache of exported dma_buf fds so
> > that it hands out the same dma_buf every time.
> 
> I agree with the reasoning here.
> 
> Though it would be nice to have this "expected driver behavior" put down
> somewhere in the documentation. Any volunteers?

I'm not sure how much sense this makes. Imo the importer side is rather
clearly spelled out, everything else is only now shaping up. And even with
the clear interface spec we pretty much have no in-tree dma_buf user who
doesn't cut corners somewhere for one or the other reason. As long the set
of exporters is fairly limited we should be fine for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[김승우]

On 2013년 06월 01일 00:29, Daniel Vetter wrote:
> On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
>> Hello Daniel,
>>
>> Thanks for your comment.
>>
>> On 2013년 05월 31일 18:14, Daniel Vetter wrote:
>>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>>>> importer private data in dma-buf attachment can be used by importer to
>>>> reimport same dma-buf.
>>>>
>>>> Seung-Woo Kim (2):
>>>>   dma-buf: add importer private data to attachment
>>>>   drm/prime: find gem object from the reimported dma-buf
>>>
>>> Self-import should already work (at least with the latest refcount
>>> fixes merged). At least the tests to check both re-import on the same
>>> drm fd and on a different all work as expected now.
>>
>> Currently, prime works well for all case including self-importing,
>> importing, and reimporting as you describe. Just, importing dma-buf from
>> other driver twice with different drm_fd, each import create its own gem
>> object even two import is done for same buffer because prime_priv is in
>> struct drm_file. This means mapping to the device is done also twice.
>> IMHO, these duplicated creations and maps are not necessary if drm can
>> find previous import in different prime_priv.
> 
> Well, that's imo a bug with the other driver. If it doesn't export
> something really simple (e.g. contiguous memory which doesn't require any
> mmio resources at all) it should have a cache of exported dma_buf fds so
> that it hands out the same dma_buf every time.

Hm, all existing dma-buf exporter including i915 driver implements its
map_dma_buf callback as allocating scatter-gather table with pages in
its buffer and calling dma_map_sg() with the sgt. With different
drm_fds, importing one dma-buf *twice*, then importer calls
dma_buf_attach() and dma_buf_map_attachment() twice at least in drm
importer because re-importing case can only checked with prime_priv in
drm_file as I described.

> 
> Or it needs to be more clever in it's dma_buf_attachment_map functions and
> lookup up a pre-existing iommu mapping.
> 
> But dealing with this in the importer is just broken.
> 
>>> Second, the dma_buf_attachment is _definitely_ the wrong place to do
>>> this. If you need iommu mapping caching, that should happen at a lower
>>> level (i.e. in the map_attachment callback somewhere of the exporter,
>>> that's what the priv field in the attachment is for). Snatching away
>>> the attachement from some random other import is certainly not the way
>>> to go - attachements are _not_ refcounted!
>>
>> Yes, attachments do not have refcount, so importer should handle and drm
>> case in my patch, importer private data is gem object and it has, of
>> course, refcount.
>>
>> And at current, exporter can not classify map_dma_buf requests of same
>> importer to same buffer with different attachment because dma_buf_attach
>> always makes new attachments. To resolve this exporter should search all
>> different attachment from same importer of dma-buf and it seems more
>> complex than importer private data to me.
>>
>> If I misunderstood something, please let me know.
> 
> Like I've said above, just fix this in the exporter. If an importer sees
> two different dma_bufs it can very well presume that it those two indeed
> point to different backing storage.

Yes, my patch does not break this concept. I just fixed case importing
_one_ dma-buf twice with different drm_fds.

> 
> This will be even more important if we attach fences two dma_bufs. If your
> broken exporter creates multiple dma_bufs each one of them will have their
> own fences attached, leading to a complete disasters. Ok, strictly
> speaking if you keep the same reservation pointer for each dma_buf it'll
> work, but that's just a detail of how you solve this in the exporter.

I can not understand about broken exporter you addressed. I don't mean
exporter makes dma-bufs from one backing storage.
While, my patch prevents not to create drm gem objects from one back
storage by importing one dma-buf with different drm-fds.

I do not believe the fix of importer is the best way, but at this
moment, I have no idea how I can fix the exporter for this issue.

Best Regards,
- Seung-Woo Kim

> 
> Cheers, Daniel
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Daniel Vetter]

On Tue, Jun 04, 2013 at 07:42:22PM +0900, 김승우 wrote:
> 
> 
> On 2013년 06월 01일 00:29, Daniel Vetter wrote:
> > On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
> >> Hello Daniel,
> >>
> >> Thanks for your comment.
> >>
> >> On 2013년 05월 31일 18:14, Daniel Vetter wrote:
> >>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> >>>> importer private data in dma-buf attachment can be used by importer to
> >>>> reimport same dma-buf.
> >>>>
> >>>> Seung-Woo Kim (2):
> >>>>   dma-buf: add importer private data to attachment
> >>>>   drm/prime: find gem object from the reimported dma-buf
> >>>
> >>> Self-import should already work (at least with the latest refcount
> >>> fixes merged). At least the tests to check both re-import on the same
> >>> drm fd and on a different all work as expected now.
> >>
> >> Currently, prime works well for all case including self-importing,
> >> importing, and reimporting as you describe. Just, importing dma-buf from
> >> other driver twice with different drm_fd, each import create its own gem
> >> object even two import is done for same buffer because prime_priv is in
> >> struct drm_file. This means mapping to the device is done also twice.
> >> IMHO, these duplicated creations and maps are not necessary if drm can
> >> find previous import in different prime_priv.
> > 
> > Well, that's imo a bug with the other driver. If it doesn't export
> > something really simple (e.g. contiguous memory which doesn't require any
> > mmio resources at all) it should have a cache of exported dma_buf fds so
> > that it hands out the same dma_buf every time.
> 
> Hm, all existing dma-buf exporter including i915 driver implements its
> map_dma_buf callback as allocating scatter-gather table with pages in
> its buffer and calling dma_map_sg() with the sgt. With different
> drm_fds, importing one dma-buf *twice*, then importer calls
> dma_buf_attach() and dma_buf_map_attachment() twice at least in drm
> importer because re-importing case can only checked with prime_priv in
> drm_file as I described.

Well, but thanks to all the self-import and re-import checks, it's
_impossible_ to import the same dma_buf twice without noticing (presuming
both importer and exporter are drm devices).
> 
> > 
> > Or it needs to be more clever in it's dma_buf_attachment_map functions and
> > lookup up a pre-existing iommu mapping.
> > 
> > But dealing with this in the importer is just broken.
> > 
> >>> Second, the dma_buf_attachment is _definitely_ the wrong place to do
> >>> this. If you need iommu mapping caching, that should happen at a lower
> >>> level (i.e. in the map_attachment callback somewhere of the exporter,
> >>> that's what the priv field in the attachment is for). Snatching away
> >>> the attachement from some random other import is certainly not the way
> >>> to go - attachements are _not_ refcounted!
> >>
> >> Yes, attachments do not have refcount, so importer should handle and drm
> >> case in my patch, importer private data is gem object and it has, of
> >> course, refcount.
> >>
> >> And at current, exporter can not classify map_dma_buf requests of same
> >> importer to same buffer with different attachment because dma_buf_attach
> >> always makes new attachments. To resolve this exporter should search all
> >> different attachment from same importer of dma-buf and it seems more
> >> complex than importer private data to me.
> >>
> >> If I misunderstood something, please let me know.
> > 
> > Like I've said above, just fix this in the exporter. If an importer sees
> > two different dma_bufs it can very well presume that it those two indeed
> > point to different backing storage.
> 
> Yes, my patch does not break this concept. I just fixed case importing
> _one_ dma-buf twice with different drm_fds.

See above, if you have two different struct file * for the same underlying
buffer object something is wrong already.

> > This will be even more important if we attach fences two dma_bufs. If your
> > broken exporter creates multiple dma_bufs each one of them will have their
> > own fences attached, leading to a complete disasters. Ok, strictly
> > speaking if you keep the same reservation pointer for each dma_buf it'll
> > work, but that's just a detail of how you solve this in the exporter.
> 
> I can not understand about broken exporter you addressed. I don't mean
> exporter makes dma-bufs from one backing storage.
> While, my patch prevents not to create drm gem objects from one back
> storage by importing one dma-buf with different drm-fds.

Well, we also have code in drm prime for that case - if the same dma_buf
object shows up multiple times, we'll only import it once. For the second
import we'll return the already created drm_gem object from the first
import, but with the refcount incremented.

> I do not believe the fix of importer is the best way, but at this
> moment, I have no idea how I can fix the exporter for this issue.

I think if you have drm prime drivers both as importers and exporters, it
is already fixed. It is correct though that both importer and exporter
need a bit of code to take care and not accidentally duplicate a shared
object somehow.

But since you've proposed your rfc as part of the drm subsystem I've
figured that we don't need to discuss the duplicate import handling code.

Yours, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[김승우]

On 2013년 06월 04일 21:55, Daniel Vetter wrote:
> On Tue, Jun 04, 2013 at 07:42:22PM +0900, 김승우 wrote:
>>
>>
>> On 2013년 06월 01일 00:29, Daniel Vetter wrote:
>>> On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
>>>> Hello Daniel,
>>>>
>>>> Thanks for your comment.
>>>>
>>>> On 2013년 05월 31일 18:14, Daniel Vetter wrote:
>>>>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>>>>>> importer private data in dma-buf attachment can be used by importer to
>>>>>> reimport same dma-buf.
>>>>>>
>>>>>> Seung-Woo Kim (2):
>>>>>>   dma-buf: add importer private data to attachment
>>>>>>   drm/prime: find gem object from the reimported dma-buf
>>>>>
>>>>> Self-import should already work (at least with the latest refcount
>>>>> fixes merged). At least the tests to check both re-import on the same
>>>>> drm fd and on a different all work as expected now.
>>>>
>>>> Currently, prime works well for all case including self-importing,
>>>> importing, and reimporting as you describe. Just, importing dma-buf from
>>>> other driver twice with different drm_fd, each import create its own gem
>>>> object even two import is done for same buffer because prime_priv is in
>>>> struct drm_file. This means mapping to the device is done also twice.
>>>> IMHO, these duplicated creations and maps are not necessary if drm can
>>>> find previous import in different prime_priv.
>>>
>>> Well, that's imo a bug with the other driver. If it doesn't export
>>> something really simple (e.g. contiguous memory which doesn't require any
>>> mmio resources at all) it should have a cache of exported dma_buf fds so
>>> that it hands out the same dma_buf every time.
>>
>> Hm, all existing dma-buf exporter including i915 driver implements its
>> map_dma_buf callback as allocating scatter-gather table with pages in
>> its buffer and calling dma_map_sg() with the sgt. With different
>> drm_fds, importing one dma-buf *twice*, then importer calls
>> dma_buf_attach() and dma_buf_map_attachment() twice at least in drm
>> importer because re-importing case can only checked with prime_priv in
>> drm_file as I described.
> 
> Well, but thanks to all the self-import and re-import checks, it's
> _impossible_ to import the same dma_buf twice without noticing (presuming
> both importer and exporter are drm devices).

No, it is possible. Prime function, drm_gem_prime_fd_to_handle(), checks
re-import with following code.

ret = drm_prime_lookup_buf_handle(&file_priv->prime,
		dma_buf, handle);

Unfortunately, file_priv is allocated per each open of drm node so this
code can only find re-import within same drm open context.

And driver specific import functions, like drm_gem_prime_import(), only
check self-import like following code.

if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
	obj = dma_buf->priv;
	if (obj->dev == dev) {
		/* ... */
	}
}

This means some application like following can make re-import to
different gem objects.

int drm_fd1, drm_fd2, ret;
int dma_buf_fd;
struct drm_prime_handle prime1, prime2;

drm_fd1 = open(DRM_NODE, O_RDWR, 0);
drm_fd2 = open(DRM_NODE, O_RDWR, 0);

/* get some dma-buf_fd from other dma-buf exporter */
prime1.fd = dma_buf_fd;
prime2.fd = dma_buf_fd;

ret = ioctl(drm_fd1, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime1);
ret = ioctl(drm_fd2, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime2);

This will import same dma-buf twice as different GEM object because
above checking codes can not check already imported gem object from the
dma-buf.

>>
>>>
>>> Or it needs to be more clever in it's dma_buf_attachment_map functions and
>>> lookup up a pre-existing iommu mapping.
>>>
>>> But dealing with this in the importer is just broken.
>>>
>>>>> Second, the dma_buf_attachment is _definitely_ the wrong place to do
>>>>> this. If you need iommu mapping caching, that should happen at a lower
>>>>> level (i.e. in the map_attachment callback somewhere of the exporter,
>>>>> that's what the priv field in the attachment is for). Snatching away
>>>>> the attachement from some random other import is certainly not the way
>>>>> to go - attachements are _not_ refcounted!
>>>>
>>>> Yes, attachments do not have refcount, so importer should handle and drm
>>>> case in my patch, importer private data is gem object and it has, of
>>>> course, refcount.
>>>>
>>>> And at current, exporter can not classify map_dma_buf requests of same
>>>> importer to same buffer with different attachment because dma_buf_attach
>>>> always makes new attachments. To resolve this exporter should search all
>>>> different attachment from same importer of dma-buf and it seems more
>>>> complex than importer private data to me.
>>>>
>>>> If I misunderstood something, please let me know.
>>>
>>> Like I've said above, just fix this in the exporter. If an importer sees
>>> two different dma_bufs it can very well presume that it those two indeed
>>> point to different backing storage.
>>
>> Yes, my patch does not break this concept. I just fixed case importing
>> _one_ dma-buf twice with different drm_fds.
> 
> See above, if you have two different struct file * for the same underlying
> buffer object something is wrong already.

drm_fds, I described, are not dma-buf fd but fds from opening DRM_NODE.

> 
>>> This will be even more important if we attach fences two dma_bufs. If your
>>> broken exporter creates multiple dma_bufs each one of them will have their
>>> own fences attached, leading to a complete disasters. Ok, strictly
>>> speaking if you keep the same reservation pointer for each dma_buf it'll
>>> work, but that's just a detail of how you solve this in the exporter.
>>
>> I can not understand about broken exporter you addressed. I don't mean
>> exporter makes dma-bufs from one backing storage.
>> While, my patch prevents not to create drm gem objects from one back
>> storage by importing one dma-buf with different drm-fds.
> 
> Well, we also have code in drm prime for that case - if the same dma_buf
> object shows up multiple times, we'll only import it once. For the second
> import we'll return the already created drm_gem object from the first
> import, but with the refcount incremented.

I already describe import check code above and it can cause different
gem objects from one dma-buf even it works well to access real buffer.

> 
>> I do not believe the fix of importer is the best way, but at this
>> moment, I have no idea how I can fix the exporter for this issue.
> 
> I think if you have drm prime drivers both as importers and exporters, it
> is already fixed. It is correct though that both importer and exporter
> need a bit of code to take care and not accidentally duplicate a shared
> object somehow.

Dave's prime reference patch v6 fixes that junk prime handle remains in
&file_priv->prime after dma-buf is released and import/export
information is cleared to increase dma-buf f_count for each
&file_priv->prime. But issue I described still remains.

> 
> But since you've proposed your rfc as part of the drm subsystem I've
> figured that we don't need to discuss the duplicate import handling code.

IMHO, considering current state of DRM PRIME, we need to discuss about
duplicate import handling. As I already wrote, I do not believe importer
private data is the best way to resolve this. So if you have better
solution, please let me know.

Thanks and Regards,
- Seung-Woo Kim

> 
> Yours, Daniel
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting

[Daniel Vetter]

On Wed, Jun 05, 2013 at 11:52:59AM +0900, 김승우 wrote:
> 
> 
> On 2013년 06월 04일 21:55, Daniel Vetter wrote:
> > On Tue, Jun 04, 2013 at 07:42:22PM +0900, 김승우 wrote:
> >>
> >>
> >> On 2013년 06월 01일 00:29, Daniel Vetter wrote:
> >>> On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote:
> >>>> Hello Daniel,
> >>>>
> >>>> Thanks for your comment.
> >>>>
> >>>> On 2013년 05월 31일 18:14, Daniel Vetter wrote:
> >>>>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> >>>>>> importer private data in dma-buf attachment can be used by importer to
> >>>>>> reimport same dma-buf.
> >>>>>>
> >>>>>> Seung-Woo Kim (2):
> >>>>>>   dma-buf: add importer private data to attachment
> >>>>>>   drm/prime: find gem object from the reimported dma-buf
> >>>>>
> >>>>> Self-import should already work (at least with the latest refcount
> >>>>> fixes merged). At least the tests to check both re-import on the same
> >>>>> drm fd and on a different all work as expected now.
> >>>>
> >>>> Currently, prime works well for all case including self-importing,
> >>>> importing, and reimporting as you describe. Just, importing dma-buf from
> >>>> other driver twice with different drm_fd, each import create its own gem
> >>>> object even two import is done for same buffer because prime_priv is in
> >>>> struct drm_file. This means mapping to the device is done also twice.
> >>>> IMHO, these duplicated creations and maps are not necessary if drm can
> >>>> find previous import in different prime_priv.
> >>>
> >>> Well, that's imo a bug with the other driver. If it doesn't export
> >>> something really simple (e.g. contiguous memory which doesn't require any
> >>> mmio resources at all) it should have a cache of exported dma_buf fds so
> >>> that it hands out the same dma_buf every time.
> >>
> >> Hm, all existing dma-buf exporter including i915 driver implements its
> >> map_dma_buf callback as allocating scatter-gather table with pages in
> >> its buffer and calling dma_map_sg() with the sgt. With different
> >> drm_fds, importing one dma-buf *twice*, then importer calls
> >> dma_buf_attach() and dma_buf_map_attachment() twice at least in drm
> >> importer because re-importing case can only checked with prime_priv in
> >> drm_file as I described.
> > 
> > Well, but thanks to all the self-import and re-import checks, it's
> > _impossible_ to import the same dma_buf twice without noticing (presuming
> > both importer and exporter are drm devices).
> 
> No, it is possible. Prime function, drm_gem_prime_fd_to_handle(), checks
> re-import with following code.
> 
> ret = drm_prime_lookup_buf_handle(&file_priv->prime,
> 		dma_buf, handle);
> 
> Unfortunately, file_priv is allocated per each open of drm node so this
> code can only find re-import within same drm open context.
> 
> And driver specific import functions, like drm_gem_prime_import(), only
> check self-import like following code.
> 
> if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
> 	obj = dma_buf->priv;
> 	if (obj->dev == dev) {
> 		/* ... */
> 	}
> }
> 
> This means some application like following can make re-import to
> different gem objects.
> 
> int drm_fd1, drm_fd2, ret;
> int dma_buf_fd;
> struct drm_prime_handle prime1, prime2;
> 
> drm_fd1 = open(DRM_NODE, O_RDWR, 0);
> drm_fd2 = open(DRM_NODE, O_RDWR, 0);
> 
> /* get some dma-buf_fd from other dma-buf exporter */
> prime1.fd = dma_buf_fd;
> prime2.fd = dma_buf_fd;
> 
> ret = ioctl(drm_fd1, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime1);
> ret = ioctl(drm_fd2, DRM_IOCTL_PRIME_FD_TO_HANDLE, &prime2);
> 
> This will import same dma-buf twice as different GEM object because
> above checking codes can not check already imported gem object from the
> dma-buf.

Oh right, now I understand. Somehow I've always thought we already take
care of this case, since I remember discussing it.

To fix that we need a device-global import cache similar to how we already
have one for each file_priv. I think we can reuse the same locking and
refcounting scheme, but I haven't checked. The commit messages of the past
few prime changes have fairly good explanations of the tricky stuff going
on there.

Sorry for being dense for so long, I should have checked my idea of what
the drm prime code does with reality sooner ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch