LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing.
@ 2018-06-05 19:03 Eric Anholt
  2018-06-05 19:03 ` [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation Eric Anholt
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Eric Anholt @ 2018-06-05 19:03 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, Lucas Stach, amd-gfx, Eric Anholt

Between creation and queueing of a job, you need to prevent any other
job from being created and queued.  Otherwise the scheduler's fences
may be signaled out of seqno order.

Signed-off-by: Eric Anholt <eric@anholt.net>
Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
---

ccing amd-gfx due to interaction of this series with the scheduler.

 drivers/gpu/drm/v3d/v3d_drv.h |  5 +++++
 drivers/gpu/drm/v3d/v3d_gem.c | 11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index a043ac3aae98..26005abd9c5d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -85,6 +85,11 @@ struct v3d_dev {
 	 */
 	struct mutex reset_lock;
 
+	/* Lock taken when creating and pushing the GPU scheduler
+	 * jobs, to keep the sched-fence seqnos in order.
+	 */
+	struct mutex sched_lock;
+
 	struct {
 		u32 num_allocated;
 		u32 pages_allocated;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index b513f9189caf..9ea83bdb9a30 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -550,13 +550,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
+	mutex_lock(&v3d->sched_lock);
 	if (exec->bin.start != exec->bin.end) {
 		ret = drm_sched_job_init(&exec->bin.base,
 					 &v3d->queue[V3D_BIN].sched,
 					 &v3d_priv->sched_entity[V3D_BIN],
 					 v3d_priv);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&v3d->sched_lock);
 			goto fail_unreserve;
+		}
 
 		exec->bin_done_fence =
 			dma_fence_get(&exec->bin.base.s_fence->finished);
@@ -570,12 +573,15 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 				 &v3d->queue[V3D_RENDER].sched,
 				 &v3d_priv->sched_entity[V3D_RENDER],
 				 v3d_priv);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&v3d->sched_lock);
 		goto fail_unreserve;
+	}
 
 	kref_get(&exec->refcount); /* put by scheduler job completion */
 	drm_sched_entity_push_job(&exec->render.base,
 				  &v3d_priv->sched_entity[V3D_RENDER]);
+	mutex_unlock(&v3d->sched_lock);
 
 	v3d_attach_object_fences(exec);
 
@@ -615,6 +621,7 @@ v3d_gem_init(struct drm_device *dev)
 	spin_lock_init(&v3d->job_lock);
 	mutex_init(&v3d->bo_lock);
 	mutex_init(&v3d->reset_lock);
+	mutex_init(&v3d->sched_lock);
 
 	/* Note: We don't allocate address 0.  Various bits of HW
 	 * treat 0 as special, such as the occlusion query counters
-- 
2.17.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation.
  2018-06-05 19:03 [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Eric Anholt
@ 2018-06-05 19:03 ` Eric Anholt
  2018-06-08 10:21   ` Lucas Stach
  2018-06-05 19:03 ` [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create() Eric Anholt
  2018-06-06  8:46 ` [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Lucas Stach
  2 siblings, 1 reply; 10+ messages in thread
From: Eric Anholt @ 2018-06-05 19:03 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, Lucas Stach, amd-gfx, Eric Anholt

Since our seqno value comes from a counter associated with the GPU
ring, not the entity (aka client), they'll be completed out of order.
There's actually no need for this code at all, since we don't have
enable_signaling() and thus DMA_FENCE_SIGNALED_BIT will be set before
we could be called.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/v3d/v3d_drv.h   |  1 -
 drivers/gpu/drm/v3d/v3d_fence.c | 13 ++++---------
 drivers/gpu/drm/v3d/v3d_gem.c   |  7 ++-----
 drivers/gpu/drm/v3d/v3d_irq.c   |  3 ---
 4 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 26005abd9c5d..f32ac8c98f37 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -25,7 +25,6 @@ struct v3d_queue_state {
 
 	u64 fence_context;
 	u64 emit_seqno;
-	u64 finished_seqno;
 };
 
 struct v3d_dev {
diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
index 087d49c8cb12..bfe31a89668b 100644
--- a/drivers/gpu/drm/v3d/v3d_fence.c
+++ b/drivers/gpu/drm/v3d/v3d_fence.c
@@ -40,19 +40,14 @@ static bool v3d_fence_enable_signaling(struct dma_fence *fence)
 	return true;
 }
 
-static bool v3d_fence_signaled(struct dma_fence *fence)
-{
-	struct v3d_fence *f = to_v3d_fence(fence);
-	struct v3d_dev *v3d = to_v3d_dev(f->dev);
-
-	return v3d->queue[f->queue].finished_seqno >= f->seqno;
-}
-
 const struct dma_fence_ops v3d_fence_ops = {
 	.get_driver_name = v3d_fence_get_driver_name,
 	.get_timeline_name = v3d_fence_get_timeline_name,
 	.enable_signaling = v3d_fence_enable_signaling,
-	.signaled = v3d_fence_signaled,
+	/* Each of our fences gets signaled as complete by the IRQ
+	 * handler, so we rely on the core's tracking of signaling.
+	 */
+	.signaled = NULL,
 	.wait = dma_fence_default_wait,
 	.release = dma_fence_free,
 };
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 9ea83bdb9a30..d06d6697e089 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -657,17 +657,14 @@ void
 v3d_gem_destroy(struct drm_device *dev)
 {
 	struct v3d_dev *v3d = to_v3d_dev(dev);
-	enum v3d_queue q;
 
 	v3d_sched_fini(v3d);
 
 	/* Waiting for exec to finish would need to be done before
 	 * unregistering V3D.
 	 */
-	for (q = 0; q < V3D_MAX_QUEUES; q++) {
-		WARN_ON(v3d->queue[q].emit_seqno !=
-			v3d->queue[q].finished_seqno);
-	}
+	WARN_ON(v3d->bin_job);
+	WARN_ON(v3d->render_job);
 
 	drm_mm_takedown(&v3d->mm);
 
diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 77e1fa046c10..e07514eb11b5 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -87,15 +87,12 @@ v3d_irq(int irq, void *arg)
 	}
 
 	if (intsts & V3D_INT_FLDONE) {
-		v3d->queue[V3D_BIN].finished_seqno++;
 		dma_fence_signal(v3d->bin_job->bin.done_fence);
 		status = IRQ_HANDLED;
 	}
 
 	if (intsts & V3D_INT_FRDONE) {
-		v3d->queue[V3D_RENDER].finished_seqno++;
 		dma_fence_signal(v3d->render_job->render.done_fence);
-
 		status = IRQ_HANDLED;
 	}
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create().
  2018-06-05 19:03 [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Eric Anholt
  2018-06-05 19:03 ` [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation Eric Anholt
@ 2018-06-05 19:03 ` Eric Anholt
  2018-06-08 10:24   ` Lucas Stach
  2018-06-06  8:46 ` [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Lucas Stach
  2 siblings, 1 reply; 10+ messages in thread
From: Eric Anholt @ 2018-06-05 19:03 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, Lucas Stach, amd-gfx, Eric Anholt

This isn't the first time I've had to argue to myself why the '++' was
safe.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/v3d/v3d_fence.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
index bfe31a89668b..6265e9ab4a13 100644
--- a/drivers/gpu/drm/v3d/v3d_fence.c
+++ b/drivers/gpu/drm/v3d/v3d_fence.c
@@ -3,6 +3,9 @@
 
 #include "v3d_drv.h"
 
+/* Note that V3D fences are created during v3d_job_run(), so we're
+ * already implictly locked.
+ */
 struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue queue)
 {
 	struct v3d_fence *fence;
-- 
2.17.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing.
  2018-06-05 19:03 [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Eric Anholt
  2018-06-05 19:03 ` [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation Eric Anholt
  2018-06-05 19:03 ` [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create() Eric Anholt
@ 2018-06-06  8:46 ` Lucas Stach
  2018-06-06  8:52   ` Christian König
  2018-06-06 17:48   ` [PATCH 1/3 v2] " Eric Anholt
  2 siblings, 2 replies; 10+ messages in thread
From: Lucas Stach @ 2018-06-06  8:46 UTC (permalink / raw)
  To: Eric Anholt, dri-devel; +Cc: linux-kernel, amd-gfx

Am Dienstag, den 05.06.2018, 12:03 -0700 schrieb Eric Anholt:
> Between creation and queueing of a job, you need to prevent any other
> job from being created and queued.  Otherwise the scheduler's fences
> may be signaled out of seqno order.
> 
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
> ---
> 
> ccing amd-gfx due to interaction of this series with the scheduler.
> 
>  drivers/gpu/drm/v3d/v3d_drv.h |  5 +++++
>  drivers/gpu/drm/v3d/v3d_gem.c | 11 +++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index a043ac3aae98..26005abd9c5d 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -85,6 +85,11 @@ struct v3d_dev {
> >  	 */
> >  	struct mutex reset_lock;
>  
> > +	/* Lock taken when creating and pushing the GPU scheduler
> > +	 * jobs, to keep the sched-fence seqnos in order.
> > +	 */
> > +	struct mutex sched_lock;
> +
> >  	struct {
> >  		u32 num_allocated;
> >  		u32 pages_allocated;
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index b513f9189caf..9ea83bdb9a30 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -550,13 +550,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> >  	if (ret)
> >  		goto fail;
>  
> > +	mutex_lock(&v3d->sched_lock);
> >  	if (exec->bin.start != exec->bin.end) {
> >  		ret = drm_sched_job_init(&exec->bin.base,
> >  					 &v3d->queue[V3D_BIN].sched,
> >  					 &v3d_priv->sched_entity[V3D_BIN],
> >  					 v3d_priv);
> > -		if (ret)
> > +		if (ret) {
> > +			mutex_unlock(&v3d->sched_lock);
>  			goto fail_unreserve;

I don't see any path where you would go to fail_unreserve with the
mutex not yet locked, so you could just fold the mutex_unlock into this
error path for a bit less code duplication.

Otherwise this looks fine.

Regards,
Lucas

> +		}
>  
> >  		exec->bin_done_fence =
> >  			dma_fence_get(&exec->bin.base.s_fence->finished);
> @@ -570,12 +573,15 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> >  				 &v3d->queue[V3D_RENDER].sched,
> >  				 &v3d_priv->sched_entity[V3D_RENDER],
> >  				 v3d_priv);
> > -	if (ret)
> > +	if (ret) {
> > +		mutex_unlock(&v3d->sched_lock);
> >  		goto fail_unreserve;
> > +	}
>  
> >  	kref_get(&exec->refcount); /* put by scheduler job completion */
> >  	drm_sched_entity_push_job(&exec->render.base,
> >  				  &v3d_priv->sched_entity[V3D_RENDER]);
> > +	mutex_unlock(&v3d->sched_lock);
>  
> >  	v3d_attach_object_fences(exec);
>  
> @@ -615,6 +621,7 @@ v3d_gem_init(struct drm_device *dev)
> >  	spin_lock_init(&v3d->job_lock);
> >  	mutex_init(&v3d->bo_lock);
> >  	mutex_init(&v3d->reset_lock);
> > +	mutex_init(&v3d->sched_lock);
>  
> >  	/* Note: We don't allocate address 0.  Various bits of HW
> >  	 * treat 0 as special, such as the occlusion query counters

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing.
  2018-06-06  8:46 ` [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Lucas Stach
@ 2018-06-06  8:52   ` Christian König
  2018-06-06 17:48   ` [PATCH 1/3 v2] " Eric Anholt
  1 sibling, 0 replies; 10+ messages in thread
From: Christian König @ 2018-06-06  8:52 UTC (permalink / raw)
  To: Lucas Stach, Eric Anholt, dri-devel; +Cc: linux-kernel, amd-gfx

Am 06.06.2018 um 10:46 schrieb Lucas Stach:
> Am Dienstag, den 05.06.2018, 12:03 -0700 schrieb Eric Anholt:
>> Between creation and queueing of a job, you need to prevent any other
>> job from being created and queued.  Otherwise the scheduler's fences
>> may be signaled out of seqno order.
>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
>> ---
>>
>> ccing amd-gfx due to interaction of this series with the scheduler.
>>
>>   drivers/gpu/drm/v3d/v3d_drv.h |  5 +++++
>>   drivers/gpu/drm/v3d/v3d_gem.c | 11 +++++++++--
>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
>> index a043ac3aae98..26005abd9c5d 100644
>> --- a/drivers/gpu/drm/v3d/v3d_drv.h
>> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
>> @@ -85,6 +85,11 @@ struct v3d_dev {
>>>   	 */
>>>   	struct mutex reset_lock;
>>   
>>> +	/* Lock taken when creating and pushing the GPU scheduler
>>> +	 * jobs, to keep the sched-fence seqnos in order.
>>> +	 */
>>> +	struct mutex sched_lock;
>> +
>>>   	struct {
>>>   		u32 num_allocated;
>>>   		u32 pages_allocated;
>> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
>> index b513f9189caf..9ea83bdb9a30 100644
>> --- a/drivers/gpu/drm/v3d/v3d_gem.c
>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>> @@ -550,13 +550,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>>>   	if (ret)
>>>   		goto fail;
>>   
>>> +	mutex_lock(&v3d->sched_lock);
>>>   	if (exec->bin.start != exec->bin.end) {
>>>   		ret = drm_sched_job_init(&exec->bin.base,
>>>   					 &v3d->queue[V3D_BIN].sched,
>>>   					 &v3d_priv->sched_entity[V3D_BIN],
>>>   					 v3d_priv);
>>> -		if (ret)
>>> +		if (ret) {
>>> +			mutex_unlock(&v3d->sched_lock);
>>   			goto fail_unreserve;
> I don't see any path where you would go to fail_unreserve with the
> mutex not yet locked, so you could just fold the mutex_unlock into this
> error path for a bit less code duplication.
>
> Otherwise this looks fine.

Yeah, agree that could be cleaned up.

I can't judge the correctness of the driver, but at least the scheduler 
handling looks good to me.

Regards,
Christian.

>
> Regards,
> Lucas
>
>> +		}
>>   
>>>   		exec->bin_done_fence =
>>>   			dma_fence_get(&exec->bin.base.s_fence->finished);
>> @@ -570,12 +573,15 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>>>   				 &v3d->queue[V3D_RENDER].sched,
>>>   				 &v3d_priv->sched_entity[V3D_RENDER],
>>>   				 v3d_priv);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		mutex_unlock(&v3d->sched_lock);
>>>   		goto fail_unreserve;
>>> +	}
>>   
>>>   	kref_get(&exec->refcount); /* put by scheduler job completion */
>>>   	drm_sched_entity_push_job(&exec->render.base,
>>>   				  &v3d_priv->sched_entity[V3D_RENDER]);
>>> +	mutex_unlock(&v3d->sched_lock);
>>   
>>>   	v3d_attach_object_fences(exec);
>>   
>> @@ -615,6 +621,7 @@ v3d_gem_init(struct drm_device *dev)
>>>   	spin_lock_init(&v3d->job_lock);
>>>   	mutex_init(&v3d->bo_lock);
>>>   	mutex_init(&v3d->reset_lock);
>>> +	mutex_init(&v3d->sched_lock);
>>   
>>>   	/* Note: We don't allocate address 0.  Various bits of HW
>>>   	 * treat 0 as special, such as the occlusion query counters
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3 v2] drm/v3d: Take a lock across GPU scheduler job creation and queuing.
  2018-06-06  8:46 ` [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Lucas Stach
  2018-06-06  8:52   ` Christian König
@ 2018-06-06 17:48   ` Eric Anholt
  2018-06-07  8:37     ` Lucas Stach
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Anholt @ 2018-06-06 17:48 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-kernel, Lucas Stach, amd-gfx, Eric Anholt

Between creation and queueing of a job, you need to prevent any other
job from being created and queued.  Otherwise the scheduler's fences
may be signaled out of seqno order.

v2: move mutex unlock to the error label.

Signed-off-by: Eric Anholt <eric@anholt.net>
Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
---
 drivers/gpu/drm/v3d/v3d_drv.h | 5 +++++
 drivers/gpu/drm/v3d/v3d_gem.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index a043ac3aae98..26005abd9c5d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -85,6 +85,11 @@ struct v3d_dev {
 	 */
 	struct mutex reset_lock;
 
+	/* Lock taken when creating and pushing the GPU scheduler
+	 * jobs, to keep the sched-fence seqnos in order.
+	 */
+	struct mutex sched_lock;
+
 	struct {
 		u32 num_allocated;
 		u32 pages_allocated;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index b513f9189caf..269fe16379c0 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -550,6 +550,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		goto fail;
 
+	mutex_lock(&v3d->sched_lock);
 	if (exec->bin.start != exec->bin.end) {
 		ret = drm_sched_job_init(&exec->bin.base,
 					 &v3d->queue[V3D_BIN].sched,
@@ -576,6 +577,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	kref_get(&exec->refcount); /* put by scheduler job completion */
 	drm_sched_entity_push_job(&exec->render.base,
 				  &v3d_priv->sched_entity[V3D_RENDER]);
+	mutex_unlock(&v3d->sched_lock);
 
 	v3d_attach_object_fences(exec);
 
@@ -594,6 +596,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	return 0;
 
 fail_unreserve:
+	mutex_unlock(&v3d->sched_lock);
 	v3d_unlock_bo_reservations(dev, exec, &acquire_ctx);
 fail:
 	v3d_exec_put(exec);
@@ -615,6 +618,7 @@ v3d_gem_init(struct drm_device *dev)
 	spin_lock_init(&v3d->job_lock);
 	mutex_init(&v3d->bo_lock);
 	mutex_init(&v3d->reset_lock);
+	mutex_init(&v3d->sched_lock);
 
 	/* Note: We don't allocate address 0.  Various bits of HW
 	 * treat 0 as special, such as the occlusion query counters
-- 
2.17.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3 v2] drm/v3d: Take a lock across GPU scheduler job creation and queuing.
  2018-06-06 17:48   ` [PATCH 1/3 v2] " Eric Anholt
@ 2018-06-07  8:37     ` Lucas Stach
  0 siblings, 0 replies; 10+ messages in thread
From: Lucas Stach @ 2018-06-07  8:37 UTC (permalink / raw)
  To: Eric Anholt, dri-devel; +Cc: linux-kernel, amd-gfx

Am Mittwoch, den 06.06.2018, 10:48 -0700 schrieb Eric Anholt:
> Between creation and queueing of a job, you need to prevent any other
> job from being created and queued.  Otherwise the scheduler's fences
> may be signaled out of seqno order.
> 
> v2: move mutex unlock to the error label.
> 
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/gpu/drm/v3d/v3d_drv.h | 5 +++++
>  drivers/gpu/drm/v3d/v3d_gem.c | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index a043ac3aae98..26005abd9c5d 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -85,6 +85,11 @@ struct v3d_dev {
> >  	 */
> >  	struct mutex reset_lock;
>  
> > +	/* Lock taken when creating and pushing the GPU scheduler
> > +	 * jobs, to keep the sched-fence seqnos in order.
> > +	 */
> > +	struct mutex sched_lock;
> +
> >  	struct {
> >  		u32 num_allocated;
> >  		u32 pages_allocated;
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index b513f9189caf..269fe16379c0 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -550,6 +550,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> >  	if (ret)
> >  		goto fail;
>  
> > +	mutex_lock(&v3d->sched_lock);
> >  	if (exec->bin.start != exec->bin.end) {
> >  		ret = drm_sched_job_init(&exec->bin.base,
> >  					 &v3d->queue[V3D_BIN].sched,
> @@ -576,6 +577,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> >  	kref_get(&exec->refcount); /* put by scheduler job completion */
> >  	drm_sched_entity_push_job(&exec->render.base,
> >  				  &v3d_priv->sched_entity[V3D_RENDER]);
> > +	mutex_unlock(&v3d->sched_lock);
>  
> >  	v3d_attach_object_fences(exec);
>  
> @@ -594,6 +596,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> >  	return 0;
>  
>  fail_unreserve:
> > +	mutex_unlock(&v3d->sched_lock);
> >  	v3d_unlock_bo_reservations(dev, exec, &acquire_ctx);
>  fail:
> >  	v3d_exec_put(exec);
> @@ -615,6 +618,7 @@ v3d_gem_init(struct drm_device *dev)
> >  	spin_lock_init(&v3d->job_lock);
> >  	mutex_init(&v3d->bo_lock);
> >  	mutex_init(&v3d->reset_lock);
> > +	mutex_init(&v3d->sched_lock);
>  
> >  	/* Note: We don't allocate address 0.  Various bits of HW
> >  	 * treat 0 as special, such as the occlusion query counters

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation.
  2018-06-05 19:03 ` [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation Eric Anholt
@ 2018-06-08 10:21   ` Lucas Stach
  0 siblings, 0 replies; 10+ messages in thread
From: Lucas Stach @ 2018-06-08 10:21 UTC (permalink / raw)
  To: Eric Anholt, dri-devel; +Cc: linux-kernel, amd-gfx

Am Dienstag, den 05.06.2018, 12:03 -0700 schrieb Eric Anholt:
> Since our seqno value comes from a counter associated with the GPU
> ring, not the entity (aka client), they'll be completed out of order.
> There's actually no need for this code at all, since we don't have
> enable_signaling() and thus DMA_FENCE_SIGNALED_BIT will be set before
> we could be called.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/gpu/drm/v3d/v3d_drv.h   |  1 -
>  drivers/gpu/drm/v3d/v3d_fence.c | 13 ++++---------
>  drivers/gpu/drm/v3d/v3d_gem.c   |  7 ++-----
>  drivers/gpu/drm/v3d/v3d_irq.c   |  3 ---
>  4 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h
> b/drivers/gpu/drm/v3d/v3d_drv.h
> index 26005abd9c5d..f32ac8c98f37 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -25,7 +25,6 @@ struct v3d_queue_state {
>  
>  	u64 fence_context;
>  	u64 emit_seqno;
> -	u64 finished_seqno;
>  };
>  
>  struct v3d_dev {
> diff --git a/drivers/gpu/drm/v3d/v3d_fence.c
> b/drivers/gpu/drm/v3d/v3d_fence.c
> index 087d49c8cb12..bfe31a89668b 100644
> --- a/drivers/gpu/drm/v3d/v3d_fence.c
> +++ b/drivers/gpu/drm/v3d/v3d_fence.c
> @@ -40,19 +40,14 @@ static bool v3d_fence_enable_signaling(struct
> dma_fence *fence)
>  	return true;
>  }
>  
> -static bool v3d_fence_signaled(struct dma_fence *fence)
> -{
> -	struct v3d_fence *f = to_v3d_fence(fence);
> -	struct v3d_dev *v3d = to_v3d_dev(f->dev);
> -
> -	return v3d->queue[f->queue].finished_seqno >= f->seqno;
> -}
> -
>  const struct dma_fence_ops v3d_fence_ops = {
>  	.get_driver_name = v3d_fence_get_driver_name,
>  	.get_timeline_name = v3d_fence_get_timeline_name,
>  	.enable_signaling = v3d_fence_enable_signaling,
> -	.signaled = v3d_fence_signaled,
> +	/* Each of our fences gets signaled as complete by the IRQ
> +	 * handler, so we rely on the core's tracking of signaling.
> +	 */
> +	.signaled = NULL,
>  	.wait = dma_fence_default_wait,
>  	.release = dma_fence_free,
>  };
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
> b/drivers/gpu/drm/v3d/v3d_gem.c
> index 9ea83bdb9a30..d06d6697e089 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -657,17 +657,14 @@ void
>  v3d_gem_destroy(struct drm_device *dev)
>  {
>  	struct v3d_dev *v3d = to_v3d_dev(dev);
> -	enum v3d_queue q;
>  
>  	v3d_sched_fini(v3d);
>  
>  	/* Waiting for exec to finish would need to be done before
>  	 * unregistering V3D.
>  	 */
> -	for (q = 0; q < V3D_MAX_QUEUES; q++) {
> -		WARN_ON(v3d->queue[q].emit_seqno !=
> -			v3d->queue[q].finished_seqno);
> -	}
> +	WARN_ON(v3d->bin_job);
> +	WARN_ON(v3d->render_job);
>  
>  	drm_mm_takedown(&v3d->mm);
>  
> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c
> b/drivers/gpu/drm/v3d/v3d_irq.c
> index 77e1fa046c10..e07514eb11b5 100644
> --- a/drivers/gpu/drm/v3d/v3d_irq.c
> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
> @@ -87,15 +87,12 @@ v3d_irq(int irq, void *arg)
>  	}
>  
>  	if (intsts & V3D_INT_FLDONE) {
> -		v3d->queue[V3D_BIN].finished_seqno++;
>  		dma_fence_signal(v3d->bin_job->bin.done_fence);
>  		status = IRQ_HANDLED;
>  	}
>  
>  	if (intsts & V3D_INT_FRDONE) {
> -		v3d->queue[V3D_RENDER].finished_seqno++;
>  		dma_fence_signal(v3d->render_job-
> >render.done_fence);
> -
>  		status = IRQ_HANDLED;
>  	}
>  

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create().
  2018-06-05 19:03 ` [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create() Eric Anholt
@ 2018-06-08 10:24   ` Lucas Stach
  2018-06-08 17:08     ` Eric Anholt
  0 siblings, 1 reply; 10+ messages in thread
From: Lucas Stach @ 2018-06-08 10:24 UTC (permalink / raw)
  To: Eric Anholt, dri-devel; +Cc: linux-kernel, amd-gfx

Am Dienstag, den 05.06.2018, 12:03 -0700 schrieb Eric Anholt:
> This isn't the first time I've had to argue to myself why the '++' was
> safe.

And now you need to do the same thing with me...

> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/v3d/v3d_fence.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
> index bfe31a89668b..6265e9ab4a13 100644
> --- a/drivers/gpu/drm/v3d/v3d_fence.c
> +++ b/drivers/gpu/drm/v3d/v3d_fence.c
> @@ -3,6 +3,9 @@
>  
>  #include "v3d_drv.h"
>  
> +/* Note that V3D fences are created during v3d_job_run(), so we're
> + * already implictly locked.
> + */
I don't see where you would be locked in the job_run path. I think what
you mean is that this path needs no locks, as it is driven by a single
scheduler thread, right?

Regards,
Lucas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create().
  2018-06-08 10:24   ` Lucas Stach
@ 2018-06-08 17:08     ` Eric Anholt
  0 siblings, 0 replies; 10+ messages in thread
From: Eric Anholt @ 2018-06-08 17:08 UTC (permalink / raw)
  To: Lucas Stach, dri-devel; +Cc: linux-kernel, amd-gfx

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

Lucas Stach <l.stach@pengutronix.de> writes:

> Am Dienstag, den 05.06.2018, 12:03 -0700 schrieb Eric Anholt:
>> This isn't the first time I've had to argue to myself why the '++' was
>> safe.
>
> And now you need to do the same thing with me...
>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>  drivers/gpu/drm/v3d/v3d_fence.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
>> index bfe31a89668b..6265e9ab4a13 100644
>> --- a/drivers/gpu/drm/v3d/v3d_fence.c
>> +++ b/drivers/gpu/drm/v3d/v3d_fence.c
>> @@ -3,6 +3,9 @@
>>  
>>  #include "v3d_drv.h"
>>  
>> +/* Note that V3D fences are created during v3d_job_run(), so we're
>> + * already implictly locked.
>> + */
> I don't see where you would be locked in the job_run path. I think what
> you mean is that this path needs no locks, as it is driven by a single
> scheduler thread, right?

Yeah, it's only called from run_job, and run_job can't reenter.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-06-08 17:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-05 19:03 [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Eric Anholt
2018-06-05 19:03 ` [PATCH 2/3] drm/v3d: Remove the bad signaled() implementation Eric Anholt
2018-06-08 10:21   ` Lucas Stach
2018-06-05 19:03 ` [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create() Eric Anholt
2018-06-08 10:24   ` Lucas Stach
2018-06-08 17:08     ` Eric Anholt
2018-06-06  8:46 ` [PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing Lucas Stach
2018-06-06  8:52   ` Christian König
2018-06-06 17:48   ` [PATCH 1/3 v2] " Eric Anholt
2018-06-07  8:37     ` Lucas Stach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).