[v6,4/5] drm/i915: Implement end_cpu_access

Commit Message

Tiago Vignatti Dec. 16, 2015, 10:25 p.m. UTC

This function is meant to be used with dma-buf mmap, when finishing the CPU
access of the mapped pointer.

The error case should be rare to happen though, requiring the buffer become
active during the sync period and for the end_cpu_access to be interrupted. So
we use a uninterruptible mutex_lock to spit out when it ever happens.

v2: disable interruption to make sure errors are reported.
v3: update to the new end_cpu_access API.

Reviewed-by: Chris Wilson <[e-mail]>
Signed-off-by: Tiago Vignatti <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Chris Wilson Dec. 17, 2015, 8:01 a.m. UTC | #1

On Wed, Dec 16, 2015 at 08:25:36PM -0200, Tiago Vignatti wrote:
> This function is meant to be used with dma-buf mmap, when finishing the CPU
> access of the mapped pointer.
> 
> +static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
> +{
> +	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> +	struct drm_device *dev = obj->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	bool was_interruptible, write = (direction == DMA_BIDIRECTIONAL || direction == DMA_TO_DEVICE);
> +	int ret;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	was_interruptible = dev_priv->mm.interruptible;
> +	dev_priv->mm.interruptible = false;
> +
> +	ret = i915_gem_object_set_to_gtt_domain(obj, write);

This only needs to pass .write=false. The dma-buf direction is
only for the period of the user access, and we are now flushing the
caches. This is equivalent to the sw-finish ioctl and ideally we just
want the i915_gem_object_flush_cpu_write_domain().
-Chris

Tiago Vignatti Dec. 18, 2015, 7:02 p.m. UTC | #2

On 12/17/2015 06:01 AM, Chris Wilson wrote:
> On Wed, Dec 16, 2015 at 08:25:36PM -0200, Tiago Vignatti wrote:
>> This function is meant to be used with dma-buf mmap, when finishing the CPU
>> access of the mapped pointer.
>>
>> +static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
>> +{
>> +	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>> +	struct drm_device *dev = obj->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	bool was_interruptible, write = (direction == DMA_BIDIRECTIONAL || direction == DMA_TO_DEVICE);
>> +	int ret;
>> +
>> +	mutex_lock(&dev->struct_mutex);
>> +	was_interruptible = dev_priv->mm.interruptible;
>> +	dev_priv->mm.interruptible = false;
>> +
>> +	ret = i915_gem_object_set_to_gtt_domain(obj, write);
>
> This only needs to pass .write=false. The dma-buf direction is
> only for the period of the user access, and we are now flushing the
> caches. This is equivalent to the sw-finish ioctl and ideally we just
> want the i915_gem_object_flush_cpu_write_domain().

in fact the only usage so far I found for end_cpu_access is when the 
pinned buffer is scanout out. Should I pretty much copy sw-finish in 
end_cpu_access then?

Thanks,

Tiago

Tiago Vignatti Dec. 18, 2015, 7:19 p.m. UTC | #3

On 12/18/2015 05:02 PM, Tiago Vignatti wrote:
> On 12/17/2015 06:01 AM, Chris Wilson wrote:
>> On Wed, Dec 16, 2015 at 08:25:36PM -0200, Tiago Vignatti wrote:
>>> This function is meant to be used with dma-buf mmap, when finishing
>>> the CPU
>>> access of the mapped pointer.
>>>
>>> +static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum
>>> dma_data_direction direction)
>>> +{
>>> +    struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>>> +    struct drm_device *dev = obj->base.dev;
>>> +    struct drm_i915_private *dev_priv = to_i915(dev);
>>> +    bool was_interruptible, write = (direction == DMA_BIDIRECTIONAL
>>> || direction == DMA_TO_DEVICE);
>>> +    int ret;
>>> +
>>> +    mutex_lock(&dev->struct_mutex);
>>> +    was_interruptible = dev_priv->mm.interruptible;
>>> +    dev_priv->mm.interruptible = false;
>>> +
>>> +    ret = i915_gem_object_set_to_gtt_domain(obj, write);
>>
>> This only needs to pass .write=false. The dma-buf direction is
>> only for the period of the user access, and we are now flushing the
>> caches. This is equivalent to the sw-finish ioctl and ideally we just
>> want the i915_gem_object_flush_cpu_write_domain().
>
> in fact the only usage so far I found for end_cpu_access is when the
> pinned buffer is scanout out. Should I pretty much copy sw-finish in
> end_cpu_access then?

And do you think it's okay to declare 
i915_gem_object_flush_cpu_write_domain outside its file's only scope?

Tiago

Chris Wilson Dec. 22, 2015, 8:51 p.m. UTC | #4

On Fri, Dec 18, 2015 at 05:19:24PM -0200, Tiago Vignatti wrote:
> On 12/18/2015 05:02 PM, Tiago Vignatti wrote:
> >On 12/17/2015 06:01 AM, Chris Wilson wrote:
> >>On Wed, Dec 16, 2015 at 08:25:36PM -0200, Tiago Vignatti wrote:
> >>>This function is meant to be used with dma-buf mmap, when finishing
> >>>the CPU
> >>>access of the mapped pointer.
> >>>
> >>>+static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum
> >>>dma_data_direction direction)
> >>>+{
> >>>+    struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> >>>+    struct drm_device *dev = obj->base.dev;
> >>>+    struct drm_i915_private *dev_priv = to_i915(dev);
> >>>+    bool was_interruptible, write = (direction == DMA_BIDIRECTIONAL
> >>>|| direction == DMA_TO_DEVICE);
> >>>+    int ret;
> >>>+
> >>>+    mutex_lock(&dev->struct_mutex);
> >>>+    was_interruptible = dev_priv->mm.interruptible;
> >>>+    dev_priv->mm.interruptible = false;
> >>>+
> >>>+    ret = i915_gem_object_set_to_gtt_domain(obj, write);
> >>
> >>This only needs to pass .write=false. The dma-buf direction is
> >>only for the period of the user access, and we are now flushing the
> >>caches. This is equivalent to the sw-finish ioctl and ideally we just
> >>want the i915_gem_object_flush_cpu_write_domain().
> >
> >in fact the only usage so far I found for end_cpu_access is when the
> >pinned buffer is scanout out. Should I pretty much copy sw-finish in
> >end_cpu_access then?
> 
> And do you think it's okay to declare
> i915_gem_object_flush_cpu_write_domain outside its file's only
> scope?

Whilst the simplicity of just doing the flush appeals, calling
set_gtt_domain(write=false) isn't that much heavier (the difference will
be lost in the noise of any clflushing) and is going to be always correct.
Whereas just flushing the CPU domain may be a hassle for us in future.
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 65ab2bd..9dba876 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -212,6 +212,27 @@  static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_dire
 	return ret;
 }
 
+static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
+{
+	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	bool was_interruptible, write = (direction == DMA_BIDIRECTIONAL || direction == DMA_TO_DEVICE);
+	int ret;
+
+	mutex_lock(&dev->struct_mutex);
+	was_interruptible = dev_priv->mm.interruptible;
+	dev_priv->mm.interruptible = false;
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, write);
+
+	dev_priv->mm.interruptible = was_interruptible;
+	mutex_unlock(&dev->struct_mutex);
+
+	if (unlikely(ret))
+		DRM_ERROR("unable to flush buffer following CPU access; rendering may be corrupt\n");
+}
+
 static const struct dma_buf_ops i915_dmabuf_ops =  {
 	.map_dma_buf = i915_gem_map_dma_buf,
 	.unmap_dma_buf = i915_gem_unmap_dma_buf,
@@ -224,6 +245,7 @@  static const struct dma_buf_ops i915_dmabuf_ops =  {
 	.vmap = i915_gem_dmabuf_vmap,
 	.vunmap = i915_gem_dmabuf_vunmap,
 	.begin_cpu_access = i915_gem_begin_cpu_access,
+	.end_cpu_access = i915_gem_end_cpu_access,
 };
 
 struct dma_buf *i915_gem_prime_export(struct drm_device *dev,