sync_file rcu adoption and semaphore changes.

2017-03-14 Thread Dave Airlie 
Okay I've listened and people said this should use rcu, so I've
tried to work out what that looks like, and I present my first pass
at using rcu for sync_file. I'm pretty sure I've probably missed some
fundamental things here.

As to Chris's reserveration_object questions, yes it does the rcu stuff
and I suppose it does the exclusive fence (with 0 shared fences), so
we'd want to make sync_file wrap reservation objects, not semaphores,
since the main reason I'm at sync files in the first place is the file
part for sharing them between processes.

Dave.

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] amdgpu/cs: split out fence dependency checking

2017-03-14 Thread Dave Airlie 
From: Dave Airlie 

This just splits out the fence depenency checking into it's
own function to make it easier to add semaphore dependencies.

Signed-off-by: Dave Airlie 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 +++---
 1 file changed, 48 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 99424cb..4671432 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -963,56 +963,66 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
return 0;
 }
 
-static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
- struct amdgpu_cs_parser *p)
+static int amdgpu_process_fence_dep(struct amdgpu_device *adev,
+   struct amdgpu_cs_parser *p,
+   struct amdgpu_cs_chunk *chunk)
 {
struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
-   int i, j, r;
-
-   for (i = 0; i < p->nchunks; ++i) {
-   struct drm_amdgpu_cs_chunk_dep *deps;
-   struct amdgpu_cs_chunk *chunk;
-   unsigned num_deps;
+   unsigned num_deps;
+   int i, r;
+   struct drm_amdgpu_cs_chunk_dep *deps;
 
-   chunk = >chunks[i];
+   deps = (struct drm_amdgpu_cs_chunk_dep *)chunk->kdata;
+   num_deps = chunk->length_dw * 4 /
+   sizeof(struct drm_amdgpu_cs_chunk_dep);
 
-   if (chunk->chunk_id != AMDGPU_CHUNK_ID_DEPENDENCIES)
-   continue;
+   for (i = 0; i < num_deps; ++i) {
+   struct amdgpu_ring *ring;
+   struct amdgpu_ctx *ctx;
+   struct dma_fence *fence;
 
-   deps = (struct drm_amdgpu_cs_chunk_dep *)chunk->kdata;
-   num_deps = chunk->length_dw * 4 /
-   sizeof(struct drm_amdgpu_cs_chunk_dep);
+   r = amdgpu_cs_get_ring(adev, deps[i].ip_type,
+  deps[i].ip_instance,
+  deps[i].ring, );
+   if (r)
+   return r;
 
-   for (j = 0; j < num_deps; ++j) {
-   struct amdgpu_ring *ring;
-   struct amdgpu_ctx *ctx;
-   struct dma_fence *fence;
+   ctx = amdgpu_ctx_get(fpriv, deps[i].ctx_id);
+   if (ctx == NULL)
+   return -EINVAL;
 
-   r = amdgpu_cs_get_ring(adev, deps[j].ip_type,
-  deps[j].ip_instance,
-  deps[j].ring, );
+   fence = amdgpu_ctx_get_fence(ctx, ring,
+deps[i].handle);
+   if (IS_ERR(fence)) {
+   r = PTR_ERR(fence);
+   amdgpu_ctx_put(ctx);
+   return r;
+   } else if (fence) {
+   r = amdgpu_sync_fence(adev, >job->sync,
+ fence);
+   dma_fence_put(fence);
+   amdgpu_ctx_put(ctx);
if (r)
return r;
+   }
+   }
+   return 0;
+}
 
-   ctx = amdgpu_ctx_get(fpriv, deps[j].ctx_id);
-   if (ctx == NULL)
-   return -EINVAL;
+static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
+ struct amdgpu_cs_parser *p)
+{
+   int i, r;
 
-   fence = amdgpu_ctx_get_fence(ctx, ring,
-deps[j].handle);
-   if (IS_ERR(fence)) {
-   r = PTR_ERR(fence);
-   amdgpu_ctx_put(ctx);
-   return r;
+   for (i = 0; i < p->nchunks; ++i) {
+   struct amdgpu_cs_chunk *chunk;
 
-   } else if (fence) {
-   r = amdgpu_sync_fence(adev, >job->sync,
- fence);
-   dma_fence_put(fence);
-   amdgpu_ctx_put(ctx);
-   if (r)
-   return r;
-   }
+   chunk = >chunks[i];
+
+   if (chunk->chunk_id == AMDGPU_CHUNK_ID_DEPENDENCIES) {
+   r = amdgpu_process_fence_dep(adev, p, chunk);
+   if (r)
+   return r;
}
}
 
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v22)

2017-03-14 Thread Dave Airlie 
From: Dave Airlie 

This isn't needed currently, but to reuse sync file for Vulkan
permanent shared semaphore semantics, we need to be able to swap
the fence backing a sync file. This patch adds a mutex to the
sync file and uses to protect accesses to the fence and cb members.

v1.1: fix the locking (Julia Lawall).
v2: use rcu try one

Signed-off-by: Dave Airlie 
---
 drivers/dma-buf/sync_file.c | 82 +++--
 include/linux/sync_file.h   |  5 ++-
 2 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..8b34f21 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,6 +28,10 @@
 
 static const struct file_operations sync_file_fops;
 
+#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
+#define sync_file_assert_held(obj) \
+   lockdep_assert_held(&(obj)->lock)
+
 static struct sync_file *sync_file_alloc(void)
 {
struct sync_file *sync_file;
@@ -47,6 +51,9 @@ static struct sync_file *sync_file_alloc(void)
 
INIT_LIST_HEAD(_file->cb.node);
 
+   RCU_INIT_POINTER(sync_file->fence, NULL);
+
+   mutex_init(_file->lock);
return sync_file;
 
 err:
@@ -80,7 +87,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
if (!sync_file)
return NULL;
 
-   sync_file->fence = dma_fence_get(fence);
+   dma_fence_get(fence);
+
+   RCU_INIT_POINTER(sync_file->fence, fence);
 
snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 fence->ops->get_driver_name(fence),
@@ -124,13 +133,26 @@ struct dma_fence *sync_file_get_fence(int fd)
if (!sync_file)
return NULL;
 
-   fence = dma_fence_get(sync_file->fence);
+   if (!rcu_access_pointer(sync_file->fence))
+   return NULL;
+
+   rcu_read_lock();
+   fence = dma_fence_get_rcu_safe(_file->fence);
+   rcu_read_unlock();
+
fput(sync_file->file);
 
return fence;
 }
 EXPORT_SYMBOL(sync_file_get_fence);
 
+static inline struct dma_fence *
+sync_file_get_fence_locked(struct sync_file *sync_file)
+{
+   return rcu_dereference_protected(sync_file->fence,
+sync_file_held(sync_file));
+}
+
 static int sync_file_set_fence(struct sync_file *sync_file,
   struct dma_fence **fences, int num_fences)
 {
@@ -143,7 +165,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 * we own the reference of the dma_fence_array creation.
 */
if (num_fences == 1) {
-   sync_file->fence = fences[0];
+   RCU_INIT_POINTER(sync_file->fence, fences[0]);
kfree(fences);
} else {
array = dma_fence_array_create(num_fences, fences,
@@ -152,17 +174,20 @@ static int sync_file_set_fence(struct sync_file 
*sync_file,
if (!array)
return -ENOMEM;
 
-   sync_file->fence = >base;
+   RCU_INIT_POINTER(sync_file->fence, >base);
}
 
return 0;
 }
 
+/* must be called with sync_file lock taken */
 static struct dma_fence **get_fences(struct sync_file *sync_file,
 int *num_fences)
 {
-   if (dma_fence_is_array(sync_file->fence)) {
-   struct dma_fence_array *array = 
to_dma_fence_array(sync_file->fence);
+   struct dma_fence *fence = sync_file_get_fence_locked(sync_file);
+
+   if (dma_fence_is_array(fence)) {
+   struct dma_fence_array *array = to_dma_fence_array(fence);
 
*num_fences = array->num_fences;
return array->fences;
@@ -204,10 +229,13 @@ static struct sync_file *sync_file_merge(const char 
*name, struct sync_file *a,
if (!sync_file)
return NULL;
 
+   mutex_lock(>lock);
+   mutex_lock(>lock);
a_fences = get_fences(a, _num_fences);
b_fences = get_fences(b, _num_fences);
-   if (a_num_fences > INT_MAX - b_num_fences)
-   return NULL;
+   if (a_num_fences > INT_MAX - b_num_fences) {
+   goto unlock;
+   }
 
num_fences = a_num_fences + b_num_fences;
 
@@ -268,11 +296,17 @@ static struct sync_file *sync_file_merge(const char 
*name, struct sync_file *a,
goto err;
}
 
+   mutex_unlock(>lock);
+   mutex_unlock(>lock);
+
strlcpy(sync_file->name, name, sizeof(sync_file->name));
return sync_file;
 
 err:
fput(sync_file->file);
+unlock:
+   mutex_unlock(>lock);
+   mutex_unlock(>lock);
return NULL;
 
 }
@@ -281,10 +315,15 @@ static void sync_file_free(struct kref *kref)
 {
struct sync_file *sync_file = container_of(kref, struct sync_file,
 kref);
+   struct dma_fence *fence;
+
+

Re: Linux 4.11: Reported regressions as of Tuesday, 20176-03-14

2017-03-14 Thread Michel Dänzer 

[ Moving this sub-thread to the amd-gfx mailing list ]

On 14/03/17 07:02 PM, Thorsten Leemhuis wrote:
> Hi! Find below my first regression report for Linux 4.11. It lists 9
> regressions I'm currently aware of.

[...]

> Desc: DRM BUG while initializing cape verde (2nd card)
> Repo: 2017-03-13 https://bugzilla.kernel.org/show_bug.cgi?id=194867
> Stat: n/a 
> Note: patch proposed by reporter

I don't see any patch.

Looks like the amdgpu driver has never fully initialized GDS support for
SI family GPUs, and this now triggers the DRM_MM_BUG_ON which was added
to drm_mm_init in 4.11.

AMD folks, should this be addressed by fleshing out SI GDS support, or
by completely disabling GDS initialization for SI?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [rfc] amdgpu/sync_file shared semaphores

2017-03-14 Thread Dave Airlie 
On 14 March 2017 at 18:53, Daniel Vetter  wrote:
> On Tue, Mar 14, 2017 at 10:50:49AM +1000, Dave Airlie wrote:
>> This contains one libdrm patch and 4 kernel patches.
>>
>> The goal is to implement the Vulkan KHR_external_semaphore_fd interface
>> for permanent semaphore behaviour for radv.
>>
>> This code tries to enhance sync file to add the required behaviour
>> (which is mostly being able to replace the fence backing the sync file),
>> it also introduces new API to amdgpu to create/destroy/export/import the
>> sync_files which we store in an idr there, rather than fds.
>>
>> There is a possibility we should share the amdgpu_sem object tracking
>> code for other drivers, maybe we should move that to sync_file as well,
>> but I'm open to suggestions for whether this is a useful approach for
>> other drivers.
>
> Yeah, makes sense. I couldn't google the spec and didn't bother to figure
> out where my intel khronos login went, so didn't double-check precise
> semantics, just dropped a question. Few more things on the actual
> sync_file stuff itself.
>
> Really big wish here is for some igts using the debug/testing fence stuff
> we have in vgem so that we can validate the corner-cases of fence
> replacement and make sure nothing falls over. Especially with all the rcu
> dancing sync_file/dma_fence have become non-trival, exhaustive testing is
> needed here imo.

We'd have to add vgem specific interfaces to trigger the replacement
path though,
since the replacement path is only going to be used for the semaphore sematics.

Suggestions on how to test better welcome!

>
> Also, NULL sync_file->fence is probably what we want for the future fences
> (which android wants to keep tilers well-fed by essentially looping the
> entire render pipeline on itself), so this goes into the right direction
> for sure. I think, but coffee kicked in already :-)

Dave.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread Marek Olšák 
While it's nice that you are all having fun here, I don't think that's
the way to communicate this.

The truth is, if AMD had an open source driver using the semaphores
(e.g. Vulkan) and if the libdrm semaphore code was merged, Dave
wouldn't be able to change it, ever. If a dependent open source
project relies on some libdrm interface, that interface is set in
stone. So AMD wouldn't be able to change it either. Unfortunately,
such an open source project doesn't exist, so the community can assume
that this libdrm code is still in the "initial design phase". Dave has
an upper hand here, because he actually has an open source project
that will use this, while AMD doesn't (yet). However, AMD can still
negotiate some details here, i.e. do a proper review.

Marek


On Tue, Mar 14, 2017 at 7:10 PM, Christian König
 wrote:
> Am 14.03.2017 um 18:45 schrieb Harry Wentland:
>>
>> On 2017-03-14 06:04 AM, zhoucm1 wrote:
>>>
>>>
>>>
>>> On 2017年03月14日 17:20, Christian König wrote:

 Am 14.03.2017 um 09:54 schrieb Daniel Vetter:
>
> On Tue, Mar 14, 2017 at 11:30:40AM +0800, zhoucm1 wrote:
>>
>>
>> On 2017年03月14日 10:52, Dave Airlie wrote:
>>>
>>> On 14 March 2017 at 12:00, zhoucm1  wrote:

 Hi Dave,

 Could you tell me why you create your new one patch? I remember I
 send out
 our the whole implementation, Why not directly review our patches?
>>>
>>> This is patch review, I'm not sure what you are expecting in terms of
>>> direct review.
>>>
>>> The patches you sent out were reviewed by Christian, he stated he
>>> thinks this should
>>> use sync_file, I was interested to see if this was actually possible,
>>> so I just adapted
>>> the patches to check if it was possible to avoid adding a lot of amd
>>> specific code
>>> for something that isn't required to be. Hence why I've sent this as
>>> an rfc, as I want
>>> to see if others think using sync_file is a good or bad idea. We may
>>> end up going
>>> back to the non-sync_file based patches if this proves to be a bad
>>> idea, so far it
>>> doesn't look like one.
>>>
>>> I also reviewed the patches and noted it shouldn't add the
>>> wait/signal
>>> interfaces,
>>> that it should use the chunks on command submission, so while I was
>>> in
>>> there I re
>>> wrote that as well.
>>
>> Yeah, then I'm ok with this, just our internal team has used this
>> implementation, they find some gaps between yours and previous, they
>> could
>> need to change their's again, they are annoyance for this.
>
> This is why you _must_ get anything you're doing discussed in upstream
> first. Your internal teams simply do not have design authority on stuff
> like that. And yes it takes forever to get formerly proprietary
> internal-only teams to understand this, speaking from years of
> experience
> implement a proper upstream-design-first approach to feature
> development
> here at intel.


 "internal teams simply do not have design authority on stuff like that"

 Can I print that on a t-shirt and start to sell it?
>>>
>>> I guess you cannot dress it to go to office..:)
>>>
>>
>> I'd wear it to the office.
>>
>> https://www.customink.com/lab?cid=hkp0-00ay-6vjg
>
>
> I'm only at an AMD office every few years, so that's probably not worth it.
>
> Anyway it's really something we should keep in mind if we want to upstream
> things.
>
> Christian.
>
>
>>
>> Harry
>>
>>> David Zhou


 Christian.

> -Daniel



>>>
>>> ___
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu/gfx7: enable cp/rlc ints after we disable clockgating

2017-03-14 Thread Alex Deucher 
Even if we disable clockgating, we still need to make sure the
cp/rlc interrupts are enabled for powergating which might still
be enabled.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 421408e..d9799ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -3797,6 +3797,9 @@ static void gfx_v7_0_enable_cgcg(struct amdgpu_device 
*adev, bool enable)
gfx_v7_0_update_rlc(adev, tmp);
 
data |= RLC_CGCG_CGLS_CTRL__CGCG_EN_MASK | 
RLC_CGCG_CGLS_CTRL__CGLS_EN_MASK;
+   if (orig != data)
+   WREG32(mmRLC_CGCG_CGLS_CTRL, data);
+
} else {
gfx_v7_0_enable_gui_idle_interrupt(adev, false);
 
@@ -3806,11 +3809,11 @@ static void gfx_v7_0_enable_cgcg(struct amdgpu_device 
*adev, bool enable)
RREG32(mmCB_CGTT_SCLK_CTRL);
 
data &= ~(RLC_CGCG_CGLS_CTRL__CGCG_EN_MASK | 
RLC_CGCG_CGLS_CTRL__CGLS_EN_MASK);
-   }
-
-   if (orig != data)
-   WREG32(mmRLC_CGCG_CGLS_CTRL, data);
+   if (orig != data)
+   WREG32(mmRLC_CGCG_CGLS_CTRL, data);
 
+   gfx_v7_0_enable_gui_idle_interrupt(adev, true);
+   }
 }
 
 static void gfx_v7_0_enable_mgcg(struct amdgpu_device *adev, bool enable)
-- 
2.5.5

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/2] drm/amdgpu/gfx8: enable cp/rlc ints after we disable clockgating

2017-03-14 Thread Alex Deucher 
Even if we disable clockgating, we still need to make sure the
cp/rlc interrupts are enabled for powergating which might still
be enabled.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index a53e36c..c9d9913 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6258,6 +6258,8 @@ static void 
gfx_v8_0_update_coarse_grain_clock_gating(struct amdgpu_device *adev
  RLC_CGCG_CGLS_CTRL__CGLS_EN_MASK);
if (temp != data)
WREG32(mmRLC_CGCG_CGLS_CTRL, data);
+   /* enable interrupts again for PG */
+   gfx_v8_0_enable_gui_idle_interrupt(adev, true);
}
 
gfx_v8_0_wait_for_rlc_serdes(adev);
-- 
2.5.5

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread Christian König 

Am 14.03.2017 um 18:45 schrieb Harry Wentland:

On 2017-03-14 06:04 AM, zhoucm1 wrote:



On 2017年03月14日 17:20, Christian König wrote:

Am 14.03.2017 um 09:54 schrieb Daniel Vetter:

On Tue, Mar 14, 2017 at 11:30:40AM +0800, zhoucm1 wrote:


On 2017年03月14日 10:52, Dave Airlie wrote:

On 14 March 2017 at 12:00, zhoucm1  wrote:

Hi Dave,

Could you tell me why you create your new one patch? I remember I
send out
our the whole implementation, Why not directly review our patches?
This is patch review, I'm not sure what you are expecting in 
terms of

direct review.

The patches you sent out were reviewed by Christian, he stated he
thinks this should
use sync_file, I was interested to see if this was actually 
possible,

so I just adapted
the patches to check if it was possible to avoid adding a lot of amd
specific code
for something that isn't required to be. Hence why I've sent this as
an rfc, as I want
to see if others think using sync_file is a good or bad idea. We may
end up going
back to the non-sync_file based patches if this proves to be a bad
idea, so far it
doesn't look like one.

I also reviewed the patches and noted it shouldn't add the 
wait/signal

interfaces,
that it should use the chunks on command submission, so while I 
was in

there I re
wrote that as well.

Yeah, then I'm ok with this, just our internal team has used this
implementation, they find some gaps between yours and previous, they
could
need to change their's again, they are annoyance for this.

This is why you _must_ get anything you're doing discussed in upstream
first. Your internal teams simply do not have design authority on 
stuff

like that. And yes it takes forever to get formerly proprietary
internal-only teams to understand this, speaking from years of
experience
implement a proper upstream-design-first approach to feature 
development

here at intel.


"internal teams simply do not have design authority on stuff like that"

Can I print that on a t-shirt and start to sell it?

I guess you cannot dress it to go to office..:)



I'd wear it to the office.

https://www.customink.com/lab?cid=hkp0-00ay-6vjg


I'm only at an AMD office every few years, so that's probably not worth it.

Anyway it's really something we should keep in mind if we want to 
upstream things.


Christian.



Harry


David Zhou


Christian.


-Daniel





___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread Harry Wentland 

On 2017-03-14 06:04 AM, zhoucm1 wrote:



On 2017年03月14日 17:20, Christian König wrote:

Am 14.03.2017 um 09:54 schrieb Daniel Vetter:

On Tue, Mar 14, 2017 at 11:30:40AM +0800, zhoucm1 wrote:


On 2017年03月14日 10:52, Dave Airlie wrote:

On 14 March 2017 at 12:00, zhoucm1  wrote:

Hi Dave,

Could you tell me why you create your new one patch? I remember I
send out
our the whole implementation, Why not directly review our patches?

This is patch review, I'm not sure what you are expecting in terms of
direct review.

The patches you sent out were reviewed by Christian, he stated he
thinks this should
use sync_file, I was interested to see if this was actually possible,
so I just adapted
the patches to check if it was possible to avoid adding a lot of amd
specific code
for something that isn't required to be. Hence why I've sent this as
an rfc, as I want
to see if others think using sync_file is a good or bad idea. We may
end up going
back to the non-sync_file based patches if this proves to be a bad
idea, so far it
doesn't look like one.

I also reviewed the patches and noted it shouldn't add the wait/signal
interfaces,
that it should use the chunks on command submission, so while I was in
there I re
wrote that as well.

Yeah, then I'm ok with this, just our internal team has used this
implementation, they find some gaps between yours and previous, they
could
need to change their's again, they are annoyance for this.

This is why you _must_ get anything you're doing discussed in upstream
first. Your internal teams simply do not have design authority on stuff
like that. And yes it takes forever to get formerly proprietary
internal-only teams to understand this, speaking from years of
experience
implement a proper upstream-design-first approach to feature development
here at intel.


"internal teams simply do not have design authority on stuff like that"

Can I print that on a t-shirt and start to sell it?

I guess you cannot dress it to go to office..:)



I'd wear it to the office.

https://www.customink.com/lab?cid=hkp0-00ay-6vjg

Harry


David Zhou


Christian.


-Daniel





___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (fwd)

2017-03-14 Thread Julia Lawall 
Perhaps the mutex on line 410 needs to be considered on line 423.

julia

-- Forwarded message --

Hi Dave,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Dave-Airlie/sync_file-add-a-mutex-to-protect-fence-and-callback-members/20170314-155609
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
:: branch date: 52 minutes ago
:: commit date: 52 minutes ago

>> drivers/dma-buf/sync_file.c:423:2-8: preceding lock on line 410

git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout a319d478cdd641742a07f809ddb7c143a0a685e9
vim +423 drivers/dma-buf/sync_file.c

d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  404
if (copy_from_user(, (void __user *)arg, sizeof(info)))
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  405
return -EFAULT;
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  406
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  407
if (info.flags || info.pad)
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  408
return -EINVAL;
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  409
a319d478 drivers/dma-buf/sync_file.c Dave Airlie 2017-03-14 @410
mutex_lock(_file->lock);
a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05  411
fences = get_fences(sync_file, _fences);
a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05  412
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  413
/*
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  414
 * Passing num_fences = 0 means that userspace doesn't want to
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  415
 * retrieve any sync_fence_info. If num_fences = 0 we skip filling
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  416
 * sync_fence_info and return the actual number of fences on
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  417
 * info->num_fences.
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  418
 */
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  419
if (!info.num_fences)
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  420
goto no_fences;
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  421
a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05  422
if (info.num_fences < num_fences)
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 @423
return -EINVAL;
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  424
a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05  425
size = num_fences * sizeof(*fence_info);
d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28  426
fence_info = kzalloc(size, GFP_KERNEL);

:: The code at line 423 was first introduced by commit
:: d4cab38e153d62ecd502645390c0289c1b8337df staging/android: prepare 
sync_file for de-staging

:: TO: Gustavo Padovan <gustavo.pado...@collabora.co.uk>
:: CC: Greg Kroah-Hartman <gre...@linuxfoundation.org>

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/radeon: Fix GPU lockups for the R7 M270

2017-03-14 Thread Alex Deucher 
On Mon, Mar 13, 2017 at 10:42 PM, Umang Raghuvanshi  wrote:
>
>
> On 03/14/2017 01:00 AM, Alex Deucher wrote:
>
>> Does your kernel have this patch:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ef736d394e85b1bf1fd65ba5e5257b85f6c82325
>
> Yes, my kernel has this patch (this issue first occurred in v4.10).


Can you send me the dmesg output from your system?

Thanks,

Alex

>
> --
> Cheers,
> Umang Raghuvanshi.
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/radeon: Fix GPU lockups for the R7 M270

2017-03-14 Thread Umang Raghuvanshi 


On 03/14/2017 01:00 AM, Alex Deucher wrote:

> Does your kernel have this patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ef736d394e85b1bf1fd65ba5e5257b85f6c82325

Yes, my kernel has this patch (this issue first occurred in v4.10).

-- 
Cheers,
Umang Raghuvanshi.



signature.asc
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v3

2017-03-14 Thread kbuild test robot 
Hi Christian,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Christian-K-nig/PCI-add-resizeable-BAR-infrastructure-v3/20170314-163334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   lib/crc32.c:148: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:148: warning: Excess function parameter 'tab' description in 
'crc32_le_generic'
   lib/crc32.c:293: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:293: warning: Excess function parameter 'tab' description in 
'crc32_be_generic'
   lib/crc32.c:1: warning: no structured comments found
>> drivers/pci/pci.c:2951: warning: No description found for parameter 'pdev'
>> drivers/pci/pci.c:2951: warning: Excess function parameter 'dev' description 
>> in 'pci_rbar_get_possible_sizes'
   drivers/pci/pci.c:2989: warning: No description found for parameter 'pdev'
>> drivers/pci/pci.c:2989: warning: Excess function parameter 'dev' description 
>> in 'pci_rbar_get_current_size'
   drivers/pci/pci.c:3027: warning: No description found for parameter 'pdev'
>> drivers/pci/pci.c:3027: warning: Excess function parameter 'dev' description 
>> in 'pci_rbar_set_size'

vim +/pdev +2951 drivers/pci/pci.c

  2945   * @bar: BAR to query
  2946   *
  2947   * Get the possible sizes of a resizeable BAR as bitmask defined in the 
spec
  2948   * (bit 0=1MB, bit 19=512GB). Returns 0 if BAR isn't resizeable.
  2949   */
  2950  u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar)
> 2951  {
  2952  unsigned pos, nbars;
  2953  u32 ctrl, cap;
  2954  unsigned i;
  2955  
  2956  pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
  2957  if (!pos)
  2958  return 0;
  2959  
  2960  pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, );
  2961  nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> 
PCI_REBAR_CTRL_NBAR_SHIFT;
  2962  
  2963  for (i = 0; i < nbars; ++i, pos += 8) {
  2964  int bar_idx;
  2965  
  2966  pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, 
);
  2967  bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
  2968  PCI_REBAR_CTRL_BAR_IDX_SHIFT;
  2969  if (bar_idx != bar)
  2970  continue;
  2971  
  2972  pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, );
  2973  return (cap & PCI_REBAR_CTRL_SIZES_MASK) >>
  2974  PCI_REBAR_CTRL_SIZES_SHIFT;
  2975  }
  2976  
  2977  return 0;
  2978  }
  2979  
  2980  /**
  2981   * pci_rbar_get_current_size - get the current size of a BAR
  2982   * @dev: PCI device
  2983   * @bar: BAR to set size to
  2984   *
  2985   * Read the size of a BAR from the resizeable BAR config.
  2986   * Returns size if found or negativ error code.
  2987   */
  2988  int pci_rbar_get_current_size(struct pci_dev *pdev, int bar)
> 2989  {
  2990  unsigned pos, nbars;
  2991  u32 ctrl;
  2992  unsigned i;
  2993  
  2994  pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
  2995  if (!pos)
  2996  return -ENOTSUPP;
  2997  
  2998  pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, );
  2999  nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> 
PCI_REBAR_CTRL_NBAR_SHIFT;
  3000  
  3001  for (i = 0; i < nbars; ++i, pos += 8) {
  3002  int bar_idx;
  3003  
  3004  pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, 
);
  3005  bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
  3006  PCI_REBAR_CTRL_BAR_IDX_SHIFT;
  3007  if (bar_idx != bar)
  3008  continue;
  3009  
  3010  return (ctrl & PCI_REBAR_CTRL_BAR_SIZE_MASK) >>
  3011  PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
  3012  }
  3013  
  3014  return -ENOENT;
  3015  }
  3016  
  3017  /**
  3018   * pci_rbar_set_size - set a new size for a BAR
  3019   * @dev: PCI device
  3020   * @bar: BAR to set size to
  3021   * @size: new size as defined in the spec (log2(size in bytes) - 20)
  3022   *
  3023   * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
  3024   * Returns true if resizing was successful, false otherwise.
  3025   */
  3026  int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)
> 3027  {
  3028  unsigned pos, nbars;
  3029  u32 ctrl;
  3030  unsigned i;

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread zhoucm1 



On 2017年03月14日 17:20, Christian König wrote:

Am 14.03.2017 um 09:54 schrieb Daniel Vetter:

On Tue, Mar 14, 2017 at 11:30:40AM +0800, zhoucm1 wrote:


On 2017年03月14日 10:52, Dave Airlie wrote:

On 14 March 2017 at 12:00, zhoucm1  wrote:

Hi Dave,

Could you tell me why you create your new one patch? I remember I 
send out

our the whole implementation, Why not directly review our patches?

This is patch review, I'm not sure what you are expecting in terms of
direct review.

The patches you sent out were reviewed by Christian, he stated he
thinks this should
use sync_file, I was interested to see if this was actually possible,
so I just adapted
the patches to check if it was possible to avoid adding a lot of amd
specific code
for something that isn't required to be. Hence why I've sent this as
an rfc, as I want
to see if others think using sync_file is a good or bad idea. We may
end up going
back to the non-sync_file based patches if this proves to be a bad
idea, so far it
doesn't look like one.

I also reviewed the patches and noted it shouldn't add the wait/signal
interfaces,
that it should use the chunks on command submission, so while I was in
there I re
wrote that as well.

Yeah, then I'm ok with this, just our internal team has used this
implementation, they find some gaps between yours and previous, they 
could

need to change their's again, they are annoyance for this.

This is why you _must_ get anything you're doing discussed in upstream
first. Your internal teams simply do not have design authority on stuff
like that. And yes it takes forever to get formerly proprietary
internal-only teams to understand this, speaking from years of 
experience

implement a proper upstream-design-first approach to feature development
here at intel.


"internal teams simply do not have design authority on stuff like that"

Can I print that on a t-shirt and start to sell it?

I guess you cannot dress it to go to office..:)

David Zhou


Christian.


-Daniel





___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait loop timed out

2017-03-14 Thread Nath, Arindam 


>-Original Message-
>From: Deucher, Alexander
>Sent: Tuesday, March 14, 2017 1:31 AM
>To: 'Daniel Drake'; j...@8bytes.org; Suthikulpanit, Suravee; Nath, Arindam
>Cc: Chris Chiu; io...@lists.linux-foundation.org; Linux Upstreaming Team;
>amd-gfx@lists.freedesktop.org
>Subject: RE: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
>loop timed out
>
>> -Original Message-
>> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
>> Of Daniel Drake
>> Sent: Monday, March 13, 2017 3:50 PM
>> To: j...@8bytes.org
>> Cc: Chris Chiu; io...@lists.linux-foundation.org; Linux Upstreaming Team;
>> amd-gfx@lists.freedesktop.org
>> Subject: amd-iommu: can't boot with amdgpu, AMD-Vi: Completion-Wait
>> loop timed out
>>
>> Hi,
>>
>> We are unable to boot Acer Aspire E5-553G (AMD FX-9800P RADEON R7) nor
>> Acer Aspire E5-523 with standard configurations because during boot
>> the screen is flooded with the following error message over and over:
>>
>>   AMD-Vi: Completion-Wait loop timed out
>>
>> We have left the system for quite a while but the message spam does
>> not stop and the system doesn't complete the boot sequence.
>>
>> We have reproduced on Linux 4.8 and Linux 4.10.
>>
>> To avoid this, we can boot with iommu=soft or just disable the amdgpu
>> display driver.
>>
>> Looks like this may also affect HP 15-ba012no :
>> https://bugzilla.redhat.com/show_bug.cgi?id=1409201
>>
>> Earlier during boot the iommu is detected as:
>>
>> [1.274518] AMD-Vi: Found IOMMU at :00:00.2 cap 0x40
>> [1.274519] AMD-Vi: Extended features (0x37ef22294ada):
>> [1.274519]  PPR NX GT IA GA PC GA_vAPIC
>> [1.274523] AMD-Vi: Interrupt remapping enabled
>> [1.274523] AMD-Vi: virtual APIC enabled
>> [1.275144] AMD-Vi: Lazy IO/TLB flushing enabled
>> [1.276498] perf: AMD NB counters detected
>> [1.278096] LVT offset 0 assigned for vector 0x400
>> [1.278963] perf: AMD IBS detected (0x07ff)
>> [1.278977] perf: amd_iommu: Detected. (0 banks, 0 counters/bank)
>>
>> Any suggestions for how we can fix this, or get more useful debug info?
>
>+Suravee and Arindam
>
>We ran into similar issues and bisected it to commit
>b1516a14657acf81a587e9a6e733a881625eee53.  I'm not too familiar with the
>IOMMU hardware to know if this is an iommu or display driver issue yet.

Like Alex mentioned, we have not yet been able to root cause the issue. But 
another way to work-around the issue without disabling graphics driver or IOMMU 
is to use amd_iommu=fullflush kernel boot param.

Thanks,
Arindam

>
>Alex
>
>>
>> Thanks
>> Daniel
>> ___
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.

2017-03-14 Thread Christian König 

Am 14.03.2017 um 10:29 schrieb Chris Wilson:

On Tue, Mar 14, 2017 at 10:13:37AM +0100, Christian König wrote:

Am 14.03.2017 um 09:45 schrieb Daniel Vetter:

On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote:

From: Dave Airlie 

This isn't needed currently, but to reuse sync file for Vulkan
permanent shared semaphore semantics, we need to be able to swap
the fence backing a sync file. This patch adds a mutex to the
sync file and uses to protect accesses to the fence and cb members.

Signed-off-by: Dave Airlie 

We've gone to pretty great lengths to rcu-protect all the fence stuff, so
that a peek-only is entirely lockless. Can we haz the same for this pls?

Yes, just wanted to suggest the same thing.

Basically you just need the following to retrieve the fence:

while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence));

We even have a helper for that:

fence = dma_fence_get_rcu_safe(_file->fence);

(Still going to suggest using a reservation_object rather than an
exclusive-only implementation.)


Yeah, thought about that as well. But the reservation object doesn't 
seem to match the required userspace semantic.


E.g. you actually don't want more than one fence it in as far as I 
understand it.


Christian.


-Chris



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.

2017-03-14 Thread Chris Wilson 
On Tue, Mar 14, 2017 at 10:13:37AM +0100, Christian König wrote:
> Am 14.03.2017 um 09:45 schrieb Daniel Vetter:
> >On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote:
> >>From: Dave Airlie 
> >>
> >>This isn't needed currently, but to reuse sync file for Vulkan
> >>permanent shared semaphore semantics, we need to be able to swap
> >>the fence backing a sync file. This patch adds a mutex to the
> >>sync file and uses to protect accesses to the fence and cb members.
> >>
> >>Signed-off-by: Dave Airlie 
> >We've gone to pretty great lengths to rcu-protect all the fence stuff, so
> >that a peek-only is entirely lockless. Can we haz the same for this pls?
> 
> Yes, just wanted to suggest the same thing.
> 
> Basically you just need the following to retrieve the fence:
> 
> while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence));

We even have a helper for that:

fence = dma_fence_get_rcu_safe(_file->fence);

(Still going to suggest using a reservation_object rather than an
exclusive-only implementation.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors

2017-03-14 Thread kbuild test robot 
Hi Christian,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Christian-K-nig/PCI-add-resizeable-BAR-infrastructure-v3/20170314-163334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-s0-201711 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   arch/x86/pci/fixup.c: In function 'pci_amd_enable_64bit_bar':
>> arch/x86/pci/fixup.c:608:52: warning: large integer implicitly truncated to 
>> unsigned type [-Woverflow]
 r = allocate_resource(_resource, res, size, 0x1,
   ^~~
   arch/x86/pci/fixup.c:609:10: warning: large integer implicitly truncated to 
unsigned type [-Woverflow]
 0xfd, size, NULL, NULL);
 ^~~~
>> arch/x86/pci/fixup.c:617:22: warning: right shift count >= width of type 
>> [-Wshift-count-overflow]
 high = ((res->start >> 40) & 0xff) |
 ^~
   arch/x86/pci/fixup.c:618:21: warning: right shift count >= width of type 
[-Wshift-count-overflow]
  res->end + 1) >> 40) & 0xff) << 16);
^~

vim +608 arch/x86/pci/fixup.c

   602  return;
   603  
   604  res = kzalloc(sizeof(*res), GFP_KERNEL);
   605  res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH | 
IORESOURCE_MEM_64 |
   606  IORESOURCE_WINDOW;
   607  res->name = dev->bus->name;
 > 608  r = allocate_resource(_resource, res, size, 0x1,
   6090xfd, size, NULL, NULL);
   610  if (r) {
   611  kfree(res);
   612  return;
   613  }
   614  
   615  base = ((res->start >> 8) & 0xff00) | 0x3;
   616  limit = ((res->end + 1) >> 8) & 0xff00;
 > 617  high = ((res->start >> 40) & 0xff) |
   618  res->end + 1) >> 40) & 0xff) << 16);
   619  
   620  pci_write_config_dword(dev, 0x180 + i * 0x4, high);

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.

2017-03-14 Thread Christian König 

Am 14.03.2017 um 09:45 schrieb Daniel Vetter:

On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote:

From: Dave Airlie 

This isn't needed currently, but to reuse sync file for Vulkan
permanent shared semaphore semantics, we need to be able to swap
the fence backing a sync file. This patch adds a mutex to the
sync file and uses to protect accesses to the fence and cb members.

Signed-off-by: Dave Airlie 

We've gone to pretty great lengths to rcu-protect all the fence stuff, so
that a peek-only is entirely lockless. Can we haz the same for this pls?


Yes, just wanted to suggest the same thing.

Basically you just need the following to retrieve the fence:

while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence));

And then only taking a look when replacing it.


Yes I know it's probably going to be slightly nasty when you get around to
implementing the replacement logic. For just normal fences we can probably
get away with not doing an rcu dance when freeing it, since the
refcounting should protect us already.


The only tricky thing I can see is the fence_callback structure in the 
sync file. And that can be handled while holding the lock in the replace 
function.



But for the replacement you need to have an rcu-delayed fence_put on the
old fences.


Freeing fences is RCU save anyway, see the default implementation of 
fence_free().


Had to fix that in amdgpu and radeon because our private implementation 
wasn't RCU save and we run into problems because of that.


So at least that should already been taken care of.

Christian.


-Daniel

---
  drivers/dma-buf/sync_file.c | 23 +++
  include/linux/sync_file.h   |  3 +++
  2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..105f48c 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -47,6 +47,7 @@ static struct sync_file *sync_file_alloc(void)
  
  	INIT_LIST_HEAD(_file->cb.node);
  
+	mutex_init(_file->lock);

return sync_file;
  
  err:

@@ -204,10 +205,13 @@ static struct sync_file *sync_file_merge(const char 
*name, struct sync_file *a,
if (!sync_file)
return NULL;
  
+	mutex_lock(>lock);

+   mutex_lock(>lock);
a_fences = get_fences(a, _num_fences);
b_fences = get_fences(b, _num_fences);
-   if (a_num_fences > INT_MAX - b_num_fences)
-   return NULL;
+   if (a_num_fences > INT_MAX - b_num_fences) {
+   goto unlock;
+   }
  
  	num_fences = a_num_fences + b_num_fences;
  
@@ -268,11 +272,17 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,

goto err;
}
  
+	mutex_unlock(>lock);

+   mutex_unlock(>lock);
+
strlcpy(sync_file->name, name, sizeof(sync_file->name));
return sync_file;
  
  err:

fput(sync_file->file);
+unlock:
+   mutex_unlock(>lock);
+   mutex_unlock(>lock);
return NULL;
  
  }

@@ -299,16 +309,20 @@ static int sync_file_release(struct inode *inode, struct 
file *file)
  static unsigned int sync_file_poll(struct file *file, poll_table *wait)
  {
struct sync_file *sync_file = file->private_data;
+   unsigned int ret_val;
  
  	poll_wait(file, _file->wq, wait);
  
+	mutex_lock(_file->lock);

if (!test_and_set_bit(POLL_ENABLED, _file->fence->flags)) {
if (dma_fence_add_callback(sync_file->fence, _file->cb,
   fence_check_cb_func) < 0)
wake_up_all(_file->wq);
}
+   ret_val = dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+   mutex_unlock(_file->lock);
  
-	return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;

+   return ret_val;
  }
  
  static long sync_file_ioctl_merge(struct sync_file *sync_file,

@@ -393,6 +407,7 @@ static long sync_file_ioctl_fence_info(struct sync_file 
*sync_file,
if (info.flags || info.pad)
return -EINVAL;
  
+	mutex_lock(_file->lock);

fences = get_fences(sync_file, _fences);
  
  	/*

@@ -433,7 +448,7 @@ static long sync_file_ioctl_fence_info(struct sync_file 
*sync_file,
  
  out:

kfree(fence_info);
-
+   mutex_unlock(_file->lock);
return ret;
  }
  
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h

index 3e3ab84..5aef17f 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -30,6 +30,7 @@
   * @wq:   wait queue for fence signaling
   * @fence:fence with the fences in the sync_file
   * @cb:   fence callback information
+ * @lock:   mutex to protect fence/cb - used for semaphores
   */
  struct sync_file {
struct file *file;
@@ -43,6 +44,8 @@ struct sync_file {
  
  	struct dma_fence	*fence;

struct dma_fence_cb cb;
+   /*

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread Christian König 

Am 14.03.2017 um 09:54 schrieb Daniel Vetter:

On Tue, Mar 14, 2017 at 11:30:40AM +0800, zhoucm1 wrote:


On 2017年03月14日 10:52, Dave Airlie wrote:

On 14 March 2017 at 12:00, zhoucm1  wrote:

Hi Dave,

Could you tell me why you create your new one patch? I remember I send out
our the whole implementation, Why not directly review our patches?

This is patch review, I'm not sure what you are expecting in terms of
direct review.

The patches you sent out were reviewed by Christian, he stated he
thinks this should
use sync_file, I was interested to see if this was actually possible,
so I just adapted
the patches to check if it was possible to avoid adding a lot of amd
specific code
for something that isn't required to be. Hence why I've sent this as
an rfc, as I want
to see if others think using sync_file is a good or bad idea. We may
end up going
back to the non-sync_file based patches if this proves to be a bad
idea, so far it
doesn't look like one.

I also reviewed the patches and noted it shouldn't add the wait/signal
interfaces,
that it should use the chunks on command submission, so while I was in
there I re
wrote that as well.

Yeah, then I'm ok with this, just our internal team has used this
implementation, they find some gaps between yours and previous, they could
need to change their's again, they are annoyance for this.

This is why you _must_ get anything you're doing discussed in upstream
first. Your internal teams simply do not have design authority on stuff
like that. And yes it takes forever to get formerly proprietary
internal-only teams to understand this, speaking from years of experience
implement a proper upstream-design-first approach to feature development
here at intel.


"internal teams simply do not have design authority on stuff like that"

Can I print that on a t-shirt and start to sell it?

Christian.


-Daniel



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] PCI: add functionality for resizing resources v2

2017-03-14 Thread kbuild test robot 
Hi Christian,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Christian-K-nig/PCI-add-resizeable-BAR-infrastructure-v3/20170314-163334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-x077-201711 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/pci/setup-res.c: In function 'pci_resize_resource':
>> drivers/pci/setup-res.c:422:2: warning: ignoring return value of 
>> 'pci_reenable_device', declared with attribute warn_unused_result 
>> [-Wunused-result]
 pci_reenable_device(dev->bus->self);
 ^~~
   drivers/pci/setup-res.c:432:2: warning: ignoring return value of 
'pci_reenable_device', declared with attribute warn_unused_result 
[-Wunused-result]
 pci_reenable_device(dev->bus->self);
 ^~~

vim +/pci_reenable_device +422 drivers/pci/setup-res.c

   406  res->end = resource_size(res) - 1;
   407  res->start = 0;
   408  if (resno < PCI_BRIDGE_RESOURCES)
   409  pci_update_resource(dev, resno);
   410  }
   411  
   412  ret = pci_rbar_set_size(dev, resno, size);
   413  if (ret)
   414  goto error_reassign;
   415  
   416  res->end = res->start + bytes - 1;
   417  
   418  ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
   419  if (ret)
   420  goto error_resize;
   421  
 > 422  pci_reenable_device(dev->bus->self);
   423  return 0;
   424  
   425  error_resize:
   426  pci_rbar_set_size(dev, resno, old);
   427  res->end = res->start + (1ULL << (old + 20)) - 1;
   428  
   429  error_reassign:
   430  pci_assign_unassigned_bus_resources(dev->bus);

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] sync_file: add replace and export some functionality

2017-03-14 Thread Chris Wilson 
On Tue, Mar 14, 2017 at 10:50:52AM +1000, Dave Airlie wrote:
> From: Dave Airlie 
> 
> Using sync_file to back vulkan semaphores means need to replace
> the fence underlying the sync file. This replace function removes
> the callback, swaps the fence, and returns the old one. This
> also exports the alloc and fdget functionality for the semaphore
> wrapper code.

Did you think about encapsulating a reservation object?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread Daniel Vetter 
On Tue, Mar 14, 2017 at 02:16:11PM +1000, Dave Airlie wrote:
> On 14 March 2017 at 13:30, zhoucm1  wrote:
> >
> >
> > On 2017年03月14日 10:52, Dave Airlie wrote:
> >>
> >> On 14 March 2017 at 12:00, zhoucm1  wrote:
> >>>
> >>> Hi Dave,
> >>>
> >>> Could you tell me why you create your new one patch? I remember I send
> >>> out
> >>> our the whole implementation, Why not directly review our patches?
> >>
> >> This is patch review, I'm not sure what you are expecting in terms of
> >> direct review.
> >>
> >> The patches you sent out were reviewed by Christian, he stated he
> >> thinks this should
> >> use sync_file, I was interested to see if this was actually possible,
> >> so I just adapted
> >> the patches to check if it was possible to avoid adding a lot of amd
> >> specific code
> >> for something that isn't required to be. Hence why I've sent this as
> >> an rfc, as I want
> >> to see if others think using sync_file is a good or bad idea. We may
> >> end up going
> >> back to the non-sync_file based patches if this proves to be a bad
> >> idea, so far it
> >> doesn't look like one.
> >>
> >> I also reviewed the patches and noted it shouldn't add the wait/signal
> >> interfaces,
> >> that it should use the chunks on command submission, so while I was in
> >> there I re
> >> wrote that as well.
> >
> > Yeah, then I'm ok with this, just our internal team has used this
> > implementation, they find some gaps between yours and previous, they could
> > need to change their's again, they are annoyance for this.
> 
> This is unfortunate, but the more internal development done, the more
> this will happen,
> especially in areas where you might interact with the kernel. I'd
> suggest trying to
> develop stuff in the open much earlier (don't start anything in-house).
> 
> Now that we have an open vulkan driver it might be that most features
> the internal
> vulkan driver requires will eventually be wanted by us,

Yeah that's another aspect, radv is the open vulkan driver for amd hw,
which means it gets to drive uapi.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm/amdgpu: add interface for kernel semaphores

2017-03-14 Thread Daniel Vetter 
On Tue, Mar 14, 2017 at 11:30:40AM +0800, zhoucm1 wrote:
> 
> 
> On 2017年03月14日 10:52, Dave Airlie wrote:
> > On 14 March 2017 at 12:00, zhoucm1  wrote:
> > > Hi Dave,
> > > 
> > > Could you tell me why you create your new one patch? I remember I send out
> > > our the whole implementation, Why not directly review our patches?
> > This is patch review, I'm not sure what you are expecting in terms of
> > direct review.
> > 
> > The patches you sent out were reviewed by Christian, he stated he
> > thinks this should
> > use sync_file, I was interested to see if this was actually possible,
> > so I just adapted
> > the patches to check if it was possible to avoid adding a lot of amd
> > specific code
> > for something that isn't required to be. Hence why I've sent this as
> > an rfc, as I want
> > to see if others think using sync_file is a good or bad idea. We may
> > end up going
> > back to the non-sync_file based patches if this proves to be a bad
> > idea, so far it
> > doesn't look like one.
> > 
> > I also reviewed the patches and noted it shouldn't add the wait/signal
> > interfaces,
> > that it should use the chunks on command submission, so while I was in
> > there I re
> > wrote that as well.
> Yeah, then I'm ok with this, just our internal team has used this
> implementation, they find some gaps between yours and previous, they could
> need to change their's again, they are annoyance for this.

This is why you _must_ get anything you're doing discussed in upstream
first. Your internal teams simply do not have design authority on stuff
like that. And yes it takes forever to get formerly proprietary
internal-only teams to understand this, speaking from years of experience
implement a proper upstream-design-first approach to feature development
here at intel.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [rfc] amdgpu/sync_file shared semaphores

2017-03-14 Thread Daniel Vetter 
On Tue, Mar 14, 2017 at 10:50:49AM +1000, Dave Airlie wrote:
> This contains one libdrm patch and 4 kernel patches.
> 
> The goal is to implement the Vulkan KHR_external_semaphore_fd interface
> for permanent semaphore behaviour for radv.
> 
> This code tries to enhance sync file to add the required behaviour
> (which is mostly being able to replace the fence backing the sync file),
> it also introduces new API to amdgpu to create/destroy/export/import the
> sync_files which we store in an idr there, rather than fds.
> 
> There is a possibility we should share the amdgpu_sem object tracking
> code for other drivers, maybe we should move that to sync_file as well,
> but I'm open to suggestions for whether this is a useful approach for
> other drivers.

Yeah, makes sense. I couldn't google the spec and didn't bother to figure
out where my intel khronos login went, so didn't double-check precise
semantics, just dropped a question. Few more things on the actual
sync_file stuff itself.

Really big wish here is for some igts using the debug/testing fence stuff
we have in vgem so that we can validate the corner-cases of fence
replacement and make sure nothing falls over. Especially with all the rcu
dancing sync_file/dma_fence have become non-trival, exhaustive testing is
needed here imo.

Also, NULL sync_file->fence is probably what we want for the future fences
(which android wants to keep tilers well-fed by essentially looping the
entire render pipeline on itself), so this goes into the right direction
for sure. I think, but coffee kicked in already :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] sync_file: add replace and export some functionality

2017-03-14 Thread Daniel Vetter 
On Tue, Mar 14, 2017 at 10:50:52AM +1000, Dave Airlie wrote:
> From: Dave Airlie 
> 
> Using sync_file to back vulkan semaphores means need to replace
> the fence underlying the sync file. This replace function removes
> the callback, swaps the fence, and returns the old one. This
> also exports the alloc and fdget functionality for the semaphore
> wrapper code.
> 
> Signed-off-by: Dave Airlie 
> ---
>  drivers/dma-buf/sync_file.c | 46 
> +
>  include/linux/sync_file.h   |  5 -
>  2 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 105f48c..df7bb37 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,7 +28,14 @@
>  
>  static const struct file_operations sync_file_fops;
>  
> -static struct sync_file *sync_file_alloc(void)
> +/**
> + * sync_file_alloc() - allocate an unfenced sync file
> + *
> + * Creates a sync_file.
> + * The sync_file can be released with fput(sync_file->file).
> + * Returns the sync_file or NULL in case of error.
> + */
> +struct sync_file *sync_file_alloc(void)
>  {
>   struct sync_file *sync_file;
>  
> @@ -54,6 +61,7 @@ static struct sync_file *sync_file_alloc(void)
>   kfree(sync_file);
>   return NULL;
>  }
> +EXPORT_SYMBOL(sync_file_alloc);
>  
>  static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
>  {
> @@ -92,7 +100,7 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
>  }
>  EXPORT_SYMBOL(sync_file_create);
>  
> -static struct sync_file *sync_file_fdget(int fd)
> +struct sync_file *sync_file_fdget(int fd)
>  {
>   struct file *file = fget(fd);
>  
> @@ -108,6 +116,7 @@ static struct sync_file *sync_file_fdget(int fd)
>   fput(file);
>   return NULL;
>  }
> +EXPORT_SYMBOL(sync_file_fdget);
>  
>  /**
>   * sync_file_get_fence - get the fence related to the sync_file fd
> @@ -125,13 +134,40 @@ struct dma_fence *sync_file_get_fence(int fd)
>   if (!sync_file)
>   return NULL;
>  
> + mutex_lock(_file->lock);
>   fence = dma_fence_get(sync_file->fence);
> + mutex_unlock(_file->lock);
>   fput(sync_file->file);
>  
>   return fence;
>  }
>  EXPORT_SYMBOL(sync_file_get_fence);
>  
> +/**
> + * sync_file_replace_fence - replace the fence related to the sync_file
> + * @sync_file:sync file to replace fence in
> + * @fence: fence to replace with (or NULL for no fence).
> + * Returns previous fence.
> + */
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> +   struct dma_fence *fence)
> +{
> + struct dma_fence *ret_fence = NULL;
> + mutex_lock(_file->lock);
> + if (sync_file->fence) {
> + if (test_bit(POLL_ENABLED, _file->fence->flags))
> + dma_fence_remove_callback(sync_file->fence, 
> _file->cb);
> + ret_fence = sync_file->fence;
> + }

uabi semantics question: Should we wake up everyone when the fence gets
replaced? What's the khr semaphore expectation here?

Spec quote in the kernel-doc (if available) would be best ...

> + if (fence)
> + sync_file->fence = dma_fence_get(fence);
> + else
> + sync_file->fence = NULL;
> + mutex_unlock(_file->lock);
> + return ret_fence;
> +}
> +EXPORT_SYMBOL(sync_file_replace_fence);
> +
>  static int sync_file_set_fence(struct sync_file *sync_file,
>  struct dma_fence **fences, int num_fences)
>  {
> @@ -292,8 +328,10 @@ static void sync_file_free(struct kref *kref)
>   struct sync_file *sync_file = container_of(kref, struct sync_file,
>kref);
>  
> - if (test_bit(POLL_ENABLED, _file->fence->flags))
> - dma_fence_remove_callback(sync_file->fence, _file->cb);
> + if (sync_file->fence) {
> + if (test_bit(POLL_ENABLED, _file->fence->flags))
> + dma_fence_remove_callback(sync_file->fence, 
> _file->cb);
> + }
>   dma_fence_put(sync_file->fence);
>   kfree(sync_file);
>  }

I think you've missed _poll, won't that oops now?
-Daniel

> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 5aef17f..9511a54 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -50,7 +50,10 @@ struct sync_file {
>  
>  #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
>  
> +struct sync_file *sync_file_alloc(void);
>  struct sync_file *sync_file_create(struct dma_fence *fence);
>  struct dma_fence *sync_file_get_fence(int fd);
> -
> +struct sync_file *sync_file_fdget(int fd);
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> +   struct dma_fence *fence);
>  #endif /* _LINUX_SYNC_H */
> -- 
> 2.7.4
> 
> ___
> dri-devel

Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.

2017-03-14 Thread Daniel Vetter 
On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote:
> From: Dave Airlie 
> 
> This isn't needed currently, but to reuse sync file for Vulkan
> permanent shared semaphore semantics, we need to be able to swap
> the fence backing a sync file. This patch adds a mutex to the
> sync file and uses to protect accesses to the fence and cb members.
> 
> Signed-off-by: Dave Airlie 

We've gone to pretty great lengths to rcu-protect all the fence stuff, so
that a peek-only is entirely lockless. Can we haz the same for this pls?

Yes I know it's probably going to be slightly nasty when you get around to
implementing the replacement logic. For just normal fences we can probably
get away with not doing an rcu dance when freeing it, since the
refcounting should protect us already.

But for the replacement you need to have an rcu-delayed fence_put on the
old fences.
-Daniel
> ---
>  drivers/dma-buf/sync_file.c | 23 +++
>  include/linux/sync_file.h   |  3 +++
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2321035..105f48c 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -47,6 +47,7 @@ static struct sync_file *sync_file_alloc(void)
>  
>   INIT_LIST_HEAD(_file->cb.node);
>  
> + mutex_init(_file->lock);
>   return sync_file;
>  
>  err:
> @@ -204,10 +205,13 @@ static struct sync_file *sync_file_merge(const char 
> *name, struct sync_file *a,
>   if (!sync_file)
>   return NULL;
>  
> + mutex_lock(>lock);
> + mutex_lock(>lock);
>   a_fences = get_fences(a, _num_fences);
>   b_fences = get_fences(b, _num_fences);
> - if (a_num_fences > INT_MAX - b_num_fences)
> - return NULL;
> + if (a_num_fences > INT_MAX - b_num_fences) {
> + goto unlock;
> + }
>  
>   num_fences = a_num_fences + b_num_fences;
>  
> @@ -268,11 +272,17 @@ static struct sync_file *sync_file_merge(const char 
> *name, struct sync_file *a,
>   goto err;
>   }
>  
> + mutex_unlock(>lock);
> + mutex_unlock(>lock);
> +
>   strlcpy(sync_file->name, name, sizeof(sync_file->name));
>   return sync_file;
>  
>  err:
>   fput(sync_file->file);
> +unlock:
> + mutex_unlock(>lock);
> + mutex_unlock(>lock);
>   return NULL;
>  
>  }
> @@ -299,16 +309,20 @@ static int sync_file_release(struct inode *inode, 
> struct file *file)
>  static unsigned int sync_file_poll(struct file *file, poll_table *wait)
>  {
>   struct sync_file *sync_file = file->private_data;
> + unsigned int ret_val;
>  
>   poll_wait(file, _file->wq, wait);
>  
> + mutex_lock(_file->lock);
>   if (!test_and_set_bit(POLL_ENABLED, _file->fence->flags)) {
>   if (dma_fence_add_callback(sync_file->fence, _file->cb,
>  fence_check_cb_func) < 0)
>   wake_up_all(_file->wq);
>   }
> + ret_val = dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> + mutex_unlock(_file->lock);
>  
> - return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> + return ret_val;
>  }
>  
>  static long sync_file_ioctl_merge(struct sync_file *sync_file,
> @@ -393,6 +407,7 @@ static long sync_file_ioctl_fence_info(struct sync_file 
> *sync_file,
>   if (info.flags || info.pad)
>   return -EINVAL;
>  
> + mutex_lock(_file->lock);
>   fences = get_fences(sync_file, _fences);
>  
>   /*
> @@ -433,7 +448,7 @@ static long sync_file_ioctl_fence_info(struct sync_file 
> *sync_file,
>  
>  out:
>   kfree(fence_info);
> -
> + mutex_unlock(_file->lock);
>   return ret;
>  }
>  
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 3e3ab84..5aef17f 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -30,6 +30,7 @@
>   * @wq:  wait queue for fence signaling
>   * @fence:   fence with the fences in the sync_file
>   * @cb:  fence callback information
> + * @lock:   mutex to protect fence/cb - used for semaphores
>   */
>  struct sync_file {
>   struct file *file;
> @@ -43,6 +44,8 @@ struct sync_file {
>  
>   struct dma_fence*fence;
>   struct dma_fence_cb cb;
> + /* protects the fence pointer and cb */
> + struct mutex lock;
>  };
>  
>  #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
> -- 
> 2.7.4
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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