[PATCH 1/2] drm/sched: add DRM_SCHED_FENCE_DONT_PIPELINE flag

[PATCH 1/2] drm/sched: add DRM_SCHED_FENCE_DONT_PIPELINE flag

[Christian König]

Setting this flag on a scheduler fence prevents pipelining of jobs
depending on this fence. In other words we always insert a full CPU
round trip before dependen jobs are pushed to the pipeline.

Signed-off-by: Christian König <christian.koenig@amd.com>
CC: stable@vger.kernel.org # 5.19+
---
 drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
 include/drm/gpu_scheduler.h              | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 191c56064f19..43d337d8b153 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -385,7 +385,8 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity)
 	}
 
 	s_fence = to_drm_sched_fence(fence);
-	if (s_fence && s_fence->sched == sched) {
+	if (s_fence && s_fence->sched == sched &&
+	    !test_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &fence->flags)) {
 
 		/*
 		 * Fence is from the same scheduler, only need to wait for
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 0fca8f38bee4..f01d14b231ed 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -32,6 +32,15 @@
 
 #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
 
+/**
+ * DRM_SCHED_FENCE_DONT_PIPELINE - Prefent dependency pipelining
+ *
+ * Setting this flag on a scheduler fence prevents pipelining of jobs depending
+ * on this fence. In other words we always insert a full CPU round trip before
+ * dependen jobs are pushed to the hw queue.
+ */
+#define DRM_SCHED_FENCE_DONT_PIPELINE	DMA_FENCE_FLAG_USER_BITS
+
 struct drm_gem_object;
 
 struct drm_gpu_scheduler;
-- 
2.25.1

[PATCH 2/2] drm/amdgpu: use DRM_SCHED_FENCE_DONT_PIPELINE for VM updates

[Christian König]

Make sure that we always have a CPU round trip to let the submission
code correctly decide if a TLB flush is necessary or not.

Signed-off-by: Christian König <christian.koenig@amd.com>
CC: stable@vger.kernel.org # 5.19+
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 2b0669c464f6..69e105fa41f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -116,8 +116,15 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 				   DMA_RESV_USAGE_BOOKKEEP);
 	}
 
-	if (fence && !p->immediate)
+	if (fence && !p->immediate) {
+		/*
+		 * Most hw generations now have a separate queue for page table
+		 * updates, but when the queue is shared with userspace we need
+		 * the extra CPU round trip to correctly flush the TLB.
+		 */
+		set_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &f->flags);
 		swap(*fence, f);
+	}
 	dma_fence_put(f);
 	return 0;
 
-- 
2.25.1

Re: [PATCH 2/2] drm/amdgpu: use DRM_SCHED_FENCE_DONT_PIPELINE for VM updates

[Luben Tuikov]

Hi Christian,

On 2022-10-14 04:15, Christian König wrote:
> Make sure that we always have a CPU round trip to let the submission
> code correctly decide if a TLB flush is necessary or not.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> CC: stable@vger.kernel.org # 5.19+
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 2b0669c464f6..69e105fa41f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -116,8 +116,15 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>  				   DMA_RESV_USAGE_BOOKKEEP);
>  	}
>  
> -	if (fence && !p->immediate)
> +	if (fence && !p->immediate) {
> +		/*
> +		 * Most hw generations now have a separate queue for page table
> +		 * updates, but when the queue is shared with userspace we need
> +		 * the extra CPU round trip to correctly flush the TLB.
> +		 */
> +		set_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &f->flags);
>  		swap(*fence, f);
> +	}

Do you ever turn that bit off?

Regards
Luben

>  	dma_fence_put(f);
>  	return 0;
>  

Re: [PATCH 2/2] drm/amdgpu: use DRM_SCHED_FENCE_DONT_PIPELINE for VM updates

[Christian König]

Am 17.10.22 um 07:29 schrieb Luben Tuikov:
> Hi Christian,
>
> On 2022-10-14 04:15, Christian König wrote:
>> Make sure that we always have a CPU round trip to let the submission
>> code correctly decide if a TLB flush is necessary or not.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> CC: stable@vger.kernel.org # 5.19+
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> index 2b0669c464f6..69e105fa41f6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> @@ -116,8 +116,15 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>>   				   DMA_RESV_USAGE_BOOKKEEP);
>>   	}
>>   
>> -	if (fence && !p->immediate)
>> +	if (fence && !p->immediate) {
>> +		/*
>> +		 * Most hw generations now have a separate queue for page table
>> +		 * updates, but when the queue is shared with userspace we need
>> +		 * the extra CPU round trip to correctly flush the TLB.
>> +		 */
>> +		set_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &f->flags);
>>   		swap(*fence, f);
>> +	}
> Do you ever turn that bit off?

No, I just rely on the fact that the flags are zero initialized.

Regards,
Christian.

>
> Regards
> Luben
>
>>   	dma_fence_put(f);
>>   	return 0;
>>   

Re: [PATCH 1/2] drm/sched: add DRM_SCHED_FENCE_DONT_PIPELINE flag

[Christian König]

Gentle ping to others to get this reviewed.

Alex, this is fixing the TLB flush errors and I think we need to get it 
into -fixes ASAP.

Christian.

Am 14.10.22 um 10:15 schrieb Christian König:
> Setting this flag on a scheduler fence prevents pipelining of jobs
> depending on this fence. In other words we always insert a full CPU
> round trip before dependen jobs are pushed to the pipeline.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> CC: stable@vger.kernel.org # 5.19+
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
>   include/drm/gpu_scheduler.h              | 9 +++++++++
>   2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 191c56064f19..43d337d8b153 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -385,7 +385,8 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity)
>   	}
>   
>   	s_fence = to_drm_sched_fence(fence);
> -	if (s_fence && s_fence->sched == sched) {
> +	if (s_fence && s_fence->sched == sched &&
> +	    !test_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &fence->flags)) {
>   
>   		/*
>   		 * Fence is from the same scheduler, only need to wait for
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 0fca8f38bee4..f01d14b231ed 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -32,6 +32,15 @@
>   
>   #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
>   
> +/**
> + * DRM_SCHED_FENCE_DONT_PIPELINE - Prefent dependency pipelining
> + *
> + * Setting this flag on a scheduler fence prevents pipelining of jobs depending
> + * on this fence. In other words we always insert a full CPU round trip before
> + * dependen jobs are pushed to the hw queue.
> + */
> +#define DRM_SCHED_FENCE_DONT_PIPELINE	DMA_FENCE_FLAG_USER_BITS
> +
>   struct drm_gem_object;
>   
>   struct drm_gpu_scheduler;

Re: [PATCH 1/2] drm/sched: add DRM_SCHED_FENCE_DONT_PIPELINE flag

[Alex Deucher]

On Tue, Oct 18, 2022 at 11:11 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Gentle ping to others to get this reviewed.
>
> Alex, this is fixing the TLB flush errors and I think we need to get it
> into -fixes ASAP.
>
> Christian.
>
> Am 14.10.22 um 10:15 schrieb Christian König:
> > Setting this flag on a scheduler fence prevents pipelining of jobs
> > depending on this fence. In other words we always insert a full CPU
> > round trip before dependen jobs are pushed to the pipeline.

typo: dependen -> dependent

> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > CC: stable@vger.kernel.org # 5.19+

Please add a link to the bug as well for both patches.  With those
fixed, series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> > ---
> >   drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
> >   include/drm/gpu_scheduler.h              | 9 +++++++++
> >   2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> > index 191c56064f19..43d337d8b153 100644
> > --- a/drivers/gpu/drm/scheduler/sched_entity.c
> > +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> > @@ -385,7 +385,8 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity)
> >       }
> >
> >       s_fence = to_drm_sched_fence(fence);
> > -     if (s_fence && s_fence->sched == sched) {
> > +     if (s_fence && s_fence->sched == sched &&
> > +         !test_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &fence->flags)) {
> >
> >               /*
> >                * Fence is from the same scheduler, only need to wait for
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index 0fca8f38bee4..f01d14b231ed 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -32,6 +32,15 @@
> >
> >   #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
> >
> > +/**
> > + * DRM_SCHED_FENCE_DONT_PIPELINE - Prefent dependency pipelining
> > + *
> > + * Setting this flag on a scheduler fence prevents pipelining of jobs depending
> > + * on this fence. In other words we always insert a full CPU round trip before
> > + * dependen jobs are pushed to the hw queue.
> > + */
> > +#define DRM_SCHED_FENCE_DONT_PIPELINE        DMA_FENCE_FLAG_USER_BITS
> > +
> >   struct drm_gem_object;
> >
> >   struct drm_gpu_scheduler;
>

Re: [PATCH 1/2] drm/sched: add DRM_SCHED_FENCE_DONT_PIPELINE flag

[Luben Tuikov]

On 2022-10-18 11:37, Alex Deucher wrote:
> On Tue, Oct 18, 2022 at 11:11 AM Christian König
> <christian.koenig@amd.com> wrote:
>>
>> Gentle ping to others to get this reviewed.
>>
>> Alex, this is fixing the TLB flush errors and I think we need to get it
>> into -fixes ASAP.
>>
>> Christian.
>>
>> Am 14.10.22 um 10:15 schrieb Christian König:
>>> Setting this flag on a scheduler fence prevents pipelining of jobs
>>> depending on this fence. In other words we always insert a full CPU
>>> round trip before dependen jobs are pushed to the pipeline.
> 
> typo: dependen -> dependent
> 
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> CC: stable@vger.kernel.org # 5.19+
> 
> Please add a link to the bug as well for both patches.  With those
> fixed, series is:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Hi Christian,

Thanks for fixing this. You can add my,

Acked-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben

> 
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
>>>   include/drm/gpu_scheduler.h              | 9 +++++++++
>>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 191c56064f19..43d337d8b153 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -385,7 +385,8 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity)
>>>       }
>>>
>>>       s_fence = to_drm_sched_fence(fence);
>>> -     if (s_fence && s_fence->sched == sched) {
>>> +     if (s_fence && s_fence->sched == sched &&
>>> +         !test_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &fence->flags)) {
>>>
>>>               /*
>>>                * Fence is from the same scheduler, only need to wait for
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 0fca8f38bee4..f01d14b231ed 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -32,6 +32,15 @@
>>>
>>>   #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
>>>
>>> +/**
>>> + * DRM_SCHED_FENCE_DONT_PIPELINE - Prefent dependency pipelining
>>> + *
>>> + * Setting this flag on a scheduler fence prevents pipelining of jobs depending
>>> + * on this fence. In other words we always insert a full CPU round trip before
>>> + * dependen jobs are pushed to the hw queue.
>>> + */
>>> +#define DRM_SCHED_FENCE_DONT_PIPELINE        DMA_FENCE_FLAG_USER_BITS
>>> +
>>>   struct drm_gem_object;
>>>
>>>   struct drm_gpu_scheduler;
>>

Re: [PATCH 1/2] drm/sched: add DRM_SCHED_FENCE_DONT_PIPELINE flag

[Luben Tuikov]

On 2022-10-14 04:15, Christian König wrote:
> Setting this flag on a scheduler fence prevents pipelining of jobs
> depending on this fence. In other words we always insert a full CPU
> round trip before dependen jobs are pushed to the pipeline.

"dependent"

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> CC: stable@vger.kernel.org # 5.19+
> ---
>  drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
>  include/drm/gpu_scheduler.h              | 9 +++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 191c56064f19..43d337d8b153 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -385,7 +385,8 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity)
>  	}
>  
>  	s_fence = to_drm_sched_fence(fence);
> -	if (s_fence && s_fence->sched == sched) {
> +	if (s_fence && s_fence->sched == sched &&
> +	    !test_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &fence->flags)) {
>  
>  		/*
>  		 * Fence is from the same scheduler, only need to wait for
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 0fca8f38bee4..f01d14b231ed 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -32,6 +32,15 @@
>  
>  #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)
>  
> +/**
> + * DRM_SCHED_FENCE_DONT_PIPELINE - Prefent dependency pipelining

"Prevent"

> + *
> + * Setting this flag on a scheduler fence prevents pipelining of jobs depending
> + * on this fence. In other words we always insert a full CPU round trip before
> + * dependen jobs are pushed to the hw queue.

"dependent"

> + */
> +#define DRM_SCHED_FENCE_DONT_PIPELINE	DMA_FENCE_FLAG_USER_BITS
> +
>  struct drm_gem_object;
>  
>  struct drm_gpu_scheduler;

With those corrections,

Acked-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben

Re: [PATCH 2/2] drm/amdgpu: use DRM_SCHED_FENCE_DONT_PIPELINE for VM updates

[Luben Tuikov]

On 2022-10-17 02:27, Christian König wrote:
> Am 17.10.22 um 07:29 schrieb Luben Tuikov:
>> Hi Christian,
>>
>> On 2022-10-14 04:15, Christian König wrote:
>>> Make sure that we always have a CPU round trip to let the submission
>>> code correctly decide if a TLB flush is necessary or not.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> CC: stable@vger.kernel.org # 5.19+
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> index 2b0669c464f6..69e105fa41f6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> @@ -116,8 +116,15 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>>>   				   DMA_RESV_USAGE_BOOKKEEP);
>>>   	}
>>>   
>>> -	if (fence && !p->immediate)
>>> +	if (fence && !p->immediate) {
>>> +		/*
>>> +		 * Most hw generations now have a separate queue for page table
>>> +		 * updates, but when the queue is shared with userspace we need
>>> +		 * the extra CPU round trip to correctly flush the TLB.
>>> +		 */
>>> +		set_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &f->flags);
>>>   		swap(*fence, f);
>>> +	}
>> Do you ever turn that bit off?
> 
> No, I just rely on the fact that the flags are zero initialized.

Acked-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben