[PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Christian König]

drm_gem_plane_helper_prepare_fb() was using
drm_atomic_set_fence_for_plane() which ignores all implicit fences when an
explicit fence is already set. That's rather unfortunate when the fb still
has a kernel fence we need to wait for to avoid presenting garbage on the
screen.

So instead update the fence in the plane state directly. While at it also
take care of all potential GEM objects and not just the first one.

Also remove the now unused drm_atomic_set_fence_for_plane() function, new
drivers should probably use the atomic helpers directly.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c       | 37 ---------------
 drivers/gpu/drm/drm_gem_atomic_helper.c | 63 +++++++++++++++++++++----
 include/drm/drm_atomic_uapi.h           |  2 -
 include/drm/drm_plane.h                 |  4 +-
 4 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index c6394ba13b24..0f461261b3f3 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 }
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
 
-/**
- * drm_atomic_set_fence_for_plane - set fence for plane
- * @plane_state: atomic state object for the plane
- * @fence: dma_fence to use for the plane
- *
- * Helper to setup the plane_state fence in case it is not set yet.
- * By using this drivers doesn't need to worry if the user choose
- * implicit or explicit fencing.
- *
- * This function will not set the fence to the state if it was set
- * via explicit fencing interfaces on the atomic ioctl. In that case it will
- * drop the reference to the fence as we are not storing it anywhere.
- * Otherwise, if &drm_plane_state.fence is not set this function we just set it
- * with the received implicit fence. In both cases this function consumes a
- * reference for @fence.
- *
- * This way explicit fencing can be used to overrule implicit fencing, which is
- * important to make explicit fencing use-cases work: One example is using one
- * buffer for 2 screens with different refresh rates. Implicit fencing will
- * clamp rendering to the refresh rate of the slower screen, whereas explicit
- * fence allows 2 independent render and display loops on a single buffer. If a
- * driver allows obeys both implicit and explicit fences for plane updates, then
- * it will break all the benefits of explicit fencing.
- */
-void
-drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
-			       struct dma_fence *fence)
-{
-	if (plane_state->fence) {
-		dma_fence_put(fence);
-		return;
-	}
-
-	plane_state->fence = fence;
-}
-EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
-
 /**
  * drm_atomic_set_crtc_for_connector - set CRTC for connector
  * @conn_state: atomic state object for the connector
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index a6d89aed0bda..8fc0b42acdff 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <linux/dma-resv.h>
+#include <linux/dma-fence-chain.h>
 
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_atomic_uapi.h>
@@ -141,25 +142,67 @@
  * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
  * explicit fencing in atomic modeset updates.
  */
-int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
+int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
+				    struct drm_plane_state *state)
 {
+	struct dma_fence *fence = dma_fence_get(state->fence);
 	struct drm_gem_object *obj;
-	struct dma_fence *fence;
+	enum dma_resv_usage usage;
+	size_t i;
 	int ret;
 
 	if (!state->fb)
 		return 0;
 
-	obj = drm_gem_fb_get_obj(state->fb, 0);
-	ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
-	if (ret)
-		return ret;
-
-	/* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
-	 * to handle more fences in general for multiple BOs per fb.
+	/*
+	 * Only add the kernel fences here if there is already a fence set via
+	 * explicit fencing interfaces on the atomic ioctl.
+	 *
+	 * This way explicit fencing can be used to overrule implicit fencing,
+	 * which is important to make explicit fencing use-cases work: One
+	 * example is using one buffer for 2 screens with different refresh
+	 * rates. Implicit fencing will clamp rendering to the refresh rate of
+	 * the slower screen, whereas explicit fence allows 2 independent
+	 * render and display loops on a single buffer. If a driver allows
+	 * obeys both implicit and explicit fences for plane updates, then it
+	 * will break all the benefits of explicit fencing.
 	 */
-	drm_atomic_set_fence_for_plane(state, fence);
+	usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
+
+	for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
+		struct dma_fence *new;
+
+		obj = drm_gem_fb_get_obj(state->fb, i);
+		if (!obj)
+			continue;
+
+		ret = dma_resv_get_singleton(obj->resv, usage, &new);
+		if (ret)
+			goto error;
+
+		if (new && fence) {
+			struct dma_fence_chain *chain = dma_fence_chain_alloc();
+
+			if (!chain) {
+				ret = -ENOMEM;
+				goto error;
+			}
+
+			dma_fence_chain_init(chain, fence, new, 1);
+			fence = &chain->base;
+
+		} else if (new) {
+			fence = new;
+		}
+	}
+
+	dma_fence_put(state->fence);
+	state->fence = fence;
 	return 0;
+
+error:
+	dma_fence_put(fence);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
 
diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
index 8cec52ad1277..4c6d39d7bdb2 100644
--- a/include/drm/drm_atomic_uapi.h
+++ b/include/drm/drm_atomic_uapi.h
@@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 			      struct drm_crtc *crtc);
 void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 				 struct drm_framebuffer *fb);
-void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
-				    struct dma_fence *fence);
 int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 2628c7cde2da..89ea54652e87 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -74,9 +74,7 @@ struct drm_plane_state {
 	 *
 	 * Optional fence to wait for before scanning out @fb. The core atomic
 	 * code will set this when userspace is using explicit fencing. Do not
-	 * write this field directly for a driver's implicit fence, use
-	 * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is
-	 * preserved.
+	 * write this field directly for a driver's implicit fence.
 	 *
 	 * Drivers should store any implicit fence in this from their
 	 * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()
-- 
2.25.1

Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Daniel Vetter]

On Thu, Apr 21, 2022 at 09:10:02PM +0200, Christian König wrote:
> drm_gem_plane_helper_prepare_fb() was using
> drm_atomic_set_fence_for_plane() which ignores all implicit fences when an
> explicit fence is already set. That's rather unfortunate when the fb still
> has a kernel fence we need to wait for to avoid presenting garbage on the
> screen.
> 
> So instead update the fence in the plane state directly. While at it also
> take care of all potential GEM objects and not just the first one.
> 
> Also remove the now unused drm_atomic_set_fence_for_plane() function, new
> drivers should probably use the atomic helpers directly.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Is this enough to have amdgpu (and well others using ttm) fully rely on
this for atomic kms updates? Anything to clean up there? Would be neat to
include that in this series too if there's anything.


> ---
>  drivers/gpu/drm/drm_atomic_uapi.c       | 37 ---------------
>  drivers/gpu/drm/drm_gem_atomic_helper.c | 63 +++++++++++++++++++++----
>  include/drm/drm_atomic_uapi.h           |  2 -
>  include/drm/drm_plane.h                 |  4 +-
>  4 files changed, 54 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c6394ba13b24..0f461261b3f3 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  }
>  EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>  
> -/**
> - * drm_atomic_set_fence_for_plane - set fence for plane
> - * @plane_state: atomic state object for the plane
> - * @fence: dma_fence to use for the plane
> - *
> - * Helper to setup the plane_state fence in case it is not set yet.
> - * By using this drivers doesn't need to worry if the user choose
> - * implicit or explicit fencing.
> - *
> - * This function will not set the fence to the state if it was set
> - * via explicit fencing interfaces on the atomic ioctl. In that case it will
> - * drop the reference to the fence as we are not storing it anywhere.
> - * Otherwise, if &drm_plane_state.fence is not set this function we just set it
> - * with the received implicit fence. In both cases this function consumes a
> - * reference for @fence.
> - *
> - * This way explicit fencing can be used to overrule implicit fencing, which is
> - * important to make explicit fencing use-cases work: One example is using one
> - * buffer for 2 screens with different refresh rates. Implicit fencing will
> - * clamp rendering to the refresh rate of the slower screen, whereas explicit
> - * fence allows 2 independent render and display loops on a single buffer. If a
> - * driver allows obeys both implicit and explicit fences for plane updates, then
> - * it will break all the benefits of explicit fencing.
> - */
> -void
> -drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> -			       struct dma_fence *fence)

I was a bit on the fence with ditching this, but the only offender not
using the prepare_fb helpers is i915, and so just more reasons that i915
needs to get off its hand-rolled atomic code with its own funky dependency
handling and everything.

> -{
> -	if (plane_state->fence) {
> -		dma_fence_put(fence);
> -		return;
> -	}
> -
> -	plane_state->fence = fence;
> -}
> -EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
> -
>  /**
>   * drm_atomic_set_crtc_for_connector - set CRTC for connector
>   * @conn_state: atomic state object for the connector
> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> index a6d89aed0bda..8fc0b42acdff 100644
> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  
>  #include <linux/dma-resv.h>
> +#include <linux/dma-fence-chain.h>
>  
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_atomic_uapi.h>
> @@ -141,25 +142,67 @@
>   * See drm_atomic_set_fence_for_plane() for a discussion of implicit and

You forgot to update the kerneldoc here, and also the reference to the
same function in the IN_FENCE_FD.

I think it'd be best to put a reference to that DOC: section here, and
adjust the uapi property doc to just state that the explicit fence will
overrule implicit sync. And then maybe also mention here that USAGE_KERNEL
fences are still obeyed.

With these changes (which should make sure that all references to
drm_atomic_set_fence_for_plane() are truly gone) this is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>



>   * explicit fencing in atomic modeset updates.
>   */
> -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
> +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
> +				    struct drm_plane_state *state)
>  {
> +	struct dma_fence *fence = dma_fence_get(state->fence);
>  	struct drm_gem_object *obj;
> -	struct dma_fence *fence;
> +	enum dma_resv_usage usage;
> +	size_t i;
>  	int ret;
>  
>  	if (!state->fb)
>  		return 0;
>  
> -	obj = drm_gem_fb_get_obj(state->fb, 0);
> -	ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
> -	if (ret)
> -		return ret;
> -
> -	/* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
> -	 * to handle more fences in general for multiple BOs per fb.
> +	/*
> +	 * Only add the kernel fences here if there is already a fence set via
> +	 * explicit fencing interfaces on the atomic ioctl.
> +	 *
> +	 * This way explicit fencing can be used to overrule implicit fencing,
> +	 * which is important to make explicit fencing use-cases work: One
> +	 * example is using one buffer for 2 screens with different refresh
> +	 * rates. Implicit fencing will clamp rendering to the refresh rate of
> +	 * the slower screen, whereas explicit fence allows 2 independent
> +	 * render and display loops on a single buffer. If a driver allows
> +	 * obeys both implicit and explicit fences for plane updates, then it
> +	 * will break all the benefits of explicit fencing.
>  	 */
> -	drm_atomic_set_fence_for_plane(state, fence);
> +	usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
> +
> +	for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
> +		struct dma_fence *new;
> +
> +		obj = drm_gem_fb_get_obj(state->fb, i);
> +		if (!obj)
> +			continue;
> +
> +		ret = dma_resv_get_singleton(obj->resv, usage, &new);
> +		if (ret)
> +			goto error;
> +
> +		if (new && fence) {
> +			struct dma_fence_chain *chain = dma_fence_chain_alloc();
> +
> +			if (!chain) {
> +				ret = -ENOMEM;
> +				goto error;
> +			}
> +
> +			dma_fence_chain_init(chain, fence, new, 1);
> +			fence = &chain->base;
> +
> +		} else if (new) {
> +			fence = new;
> +		}
> +	}
> +
> +	dma_fence_put(state->fence);
> +	state->fence = fence;
>  	return 0;
> +
> +error:
> +	dma_fence_put(fence);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
>  
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 8cec52ad1277..4c6d39d7bdb2 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  			      struct drm_crtc *crtc);
>  void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  				 struct drm_framebuffer *fb);
> -void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> -				    struct dma_fence *fence);
>  int __must_check
>  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  				  struct drm_crtc *crtc);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 2628c7cde2da..89ea54652e87 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -74,9 +74,7 @@ struct drm_plane_state {
>  	 *
>  	 * Optional fence to wait for before scanning out @fb. The core atomic
>  	 * code will set this when userspace is using explicit fencing. Do not
> -	 * write this field directly for a driver's implicit fence, use
> -	 * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is
> -	 * preserved.
> +	 * write this field directly for a driver's implicit fence.
>  	 *
>  	 * Drivers should store any implicit fence in this from their
>  	 * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Christian König]

Am 27.04.22 um 18:03 schrieb Daniel Vetter:
> On Thu, Apr 21, 2022 at 09:10:02PM +0200, Christian König wrote:
>> drm_gem_plane_helper_prepare_fb() was using
>> drm_atomic_set_fence_for_plane() which ignores all implicit fences when an
>> explicit fence is already set. That's rather unfortunate when the fb still
>> has a kernel fence we need to wait for to avoid presenting garbage on the
>> screen.
>>
>> So instead update the fence in the plane state directly. While at it also
>> take care of all potential GEM objects and not just the first one.
>>
>> Also remove the now unused drm_atomic_set_fence_for_plane() function, new
>> drivers should probably use the atomic helpers directly.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> Is this enough to have amdgpu (and well others using ttm) fully rely on
> this for atomic kms updates? Anything to clean up there? Would be neat to
> include that in this series too if there's anything.

At least I strongly think so. I just haven't removed the 
dma_resv_wait_timeout() from amdgpu_dm_commit_planes() because that is 
simply not code I'm very familiar with.

>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c       | 37 ---------------
>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 63 +++++++++++++++++++++----
>>   include/drm/drm_atomic_uapi.h           |  2 -
>>   include/drm/drm_plane.h                 |  4 +-
>>   4 files changed, 54 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index c6394ba13b24..0f461261b3f3 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>>   }
>>   EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>>   
>> -/**
>> - * drm_atomic_set_fence_for_plane - set fence for plane
>> - * @plane_state: atomic state object for the plane
>> - * @fence: dma_fence to use for the plane
>> - *
>> - * Helper to setup the plane_state fence in case it is not set yet.
>> - * By using this drivers doesn't need to worry if the user choose
>> - * implicit or explicit fencing.
>> - *
>> - * This function will not set the fence to the state if it was set
>> - * via explicit fencing interfaces on the atomic ioctl. In that case it will
>> - * drop the reference to the fence as we are not storing it anywhere.
>> - * Otherwise, if &drm_plane_state.fence is not set this function we just set it
>> - * with the received implicit fence. In both cases this function consumes a
>> - * reference for @fence.
>> - *
>> - * This way explicit fencing can be used to overrule implicit fencing, which is
>> - * important to make explicit fencing use-cases work: One example is using one
>> - * buffer for 2 screens with different refresh rates. Implicit fencing will
>> - * clamp rendering to the refresh rate of the slower screen, whereas explicit
>> - * fence allows 2 independent render and display loops on a single buffer. If a
>> - * driver allows obeys both implicit and explicit fences for plane updates, then
>> - * it will break all the benefits of explicit fencing.
>> - */
>> -void
>> -drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
>> -			       struct dma_fence *fence)
> I was a bit on the fence with ditching this, but the only offender not
> using the prepare_fb helpers is i915, and so just more reasons that i915
> needs to get off its hand-rolled atomic code with its own funky dependency
> handling and everything.

Yeah, agree totally. amdgpu should now also work out of the box, I just 
didn't dared to actually enable it in the same patch.

>
>> -{
>> -	if (plane_state->fence) {
>> -		dma_fence_put(fence);
>> -		return;
>> -	}
>> -
>> -	plane_state->fence = fence;
>> -}
>> -EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
>> -
>>   /**
>>    * drm_atomic_set_crtc_for_connector - set CRTC for connector
>>    * @conn_state: atomic state object for the connector
>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> index a6d89aed0bda..8fc0b42acdff 100644
>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>   
>>   #include <linux/dma-resv.h>
>> +#include <linux/dma-fence-chain.h>
>>   
>>   #include <drm/drm_atomic_state_helper.h>
>>   #include <drm/drm_atomic_uapi.h>
>> @@ -141,25 +142,67 @@
>>    * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
> You forgot to update the kerneldoc here, and also the reference to the
> same function in the IN_FENCE_FD.
>
> I think it'd be best to put a reference to that DOC: section here, and
> adjust the uapi property doc to just state that the explicit fence will
> overrule implicit sync. And then maybe also mention here that USAGE_KERNEL
> fences are still obeyed.
>
> With these changes (which should make sure that all references to
> drm_atomic_set_fence_for_plane() are truly gone) this is
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok, going to update the doc and push this if nobody objects.

Thanks,
Christian.

>
>
>
>>    * explicit fencing in atomic modeset updates.
>>    */
>> -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
>> +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
>> +				    struct drm_plane_state *state)
>>   {
>> +	struct dma_fence *fence = dma_fence_get(state->fence);
>>   	struct drm_gem_object *obj;
>> -	struct dma_fence *fence;
>> +	enum dma_resv_usage usage;
>> +	size_t i;
>>   	int ret;
>>   
>>   	if (!state->fb)
>>   		return 0;
>>   
>> -	obj = drm_gem_fb_get_obj(state->fb, 0);
>> -	ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
>> -	 * to handle more fences in general for multiple BOs per fb.
>> +	/*
>> +	 * Only add the kernel fences here if there is already a fence set via
>> +	 * explicit fencing interfaces on the atomic ioctl.
>> +	 *
>> +	 * This way explicit fencing can be used to overrule implicit fencing,
>> +	 * which is important to make explicit fencing use-cases work: One
>> +	 * example is using one buffer for 2 screens with different refresh
>> +	 * rates. Implicit fencing will clamp rendering to the refresh rate of
>> +	 * the slower screen, whereas explicit fence allows 2 independent
>> +	 * render and display loops on a single buffer. If a driver allows
>> +	 * obeys both implicit and explicit fences for plane updates, then it
>> +	 * will break all the benefits of explicit fencing.
>>   	 */
>> -	drm_atomic_set_fence_for_plane(state, fence);
>> +	usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
>> +		struct dma_fence *new;
>> +
>> +		obj = drm_gem_fb_get_obj(state->fb, i);
>> +		if (!obj)
>> +			continue;
>> +
>> +		ret = dma_resv_get_singleton(obj->resv, usage, &new);
>> +		if (ret)
>> +			goto error;
>> +
>> +		if (new && fence) {
>> +			struct dma_fence_chain *chain = dma_fence_chain_alloc();
>> +
>> +			if (!chain) {
>> +				ret = -ENOMEM;
>> +				goto error;
>> +			}
>> +
>> +			dma_fence_chain_init(chain, fence, new, 1);
>> +			fence = &chain->base;
>> +
>> +		} else if (new) {
>> +			fence = new;
>> +		}
>> +	}
>> +
>> +	dma_fence_put(state->fence);
>> +	state->fence = fence;
>>   	return 0;
>> +
>> +error:
>> +	dma_fence_put(fence);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
>>   
>> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
>> index 8cec52ad1277..4c6d39d7bdb2 100644
>> --- a/include/drm/drm_atomic_uapi.h
>> +++ b/include/drm/drm_atomic_uapi.h
>> @@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>>   			      struct drm_crtc *crtc);
>>   void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>>   				 struct drm_framebuffer *fb);
>> -void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
>> -				    struct dma_fence *fence);
>>   int __must_check
>>   drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>   				  struct drm_crtc *crtc);
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 2628c7cde2da..89ea54652e87 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -74,9 +74,7 @@ struct drm_plane_state {
>>   	 *
>>   	 * Optional fence to wait for before scanning out @fb. The core atomic
>>   	 * code will set this when userspace is using explicit fencing. Do not
>> -	 * write this field directly for a driver's implicit fence, use
>> -	 * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is
>> -	 * preserved.
>> +	 * write this field directly for a driver's implicit fence.
>>   	 *
>>   	 * Drivers should store any implicit fence in this from their
>>   	 * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()
>> -- 
>> 2.25.1
>>

Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 8572 bytes --]

Hi

Am 21.04.22 um 21:10 schrieb Christian König:
> drm_gem_plane_helper_prepare_fb() was using
> drm_atomic_set_fence_for_plane() which ignores all implicit fences when an
> explicit fence is already set. That's rather unfortunate when the fb still
> has a kernel fence we need to wait for to avoid presenting garbage on the
> screen.
> 
> So instead update the fence in the plane state directly. While at it also
> take care of all potential GEM objects and not just the first one.
> 
> Also remove the now unused drm_atomic_set_fence_for_plane() function, new
> drivers should probably use the atomic helpers directly.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c       | 37 ---------------
>   drivers/gpu/drm/drm_gem_atomic_helper.c | 63 +++++++++++++++++++++----
>   include/drm/drm_atomic_uapi.h           |  2 -
>   include/drm/drm_plane.h                 |  4 +-
>   4 files changed, 54 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c6394ba13b24..0f461261b3f3 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>   }
>   EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>   
> -/**
> - * drm_atomic_set_fence_for_plane - set fence for plane
> - * @plane_state: atomic state object for the plane
> - * @fence: dma_fence to use for the plane
> - *
> - * Helper to setup the plane_state fence in case it is not set yet.
> - * By using this drivers doesn't need to worry if the user choose
> - * implicit or explicit fencing.
> - *
> - * This function will not set the fence to the state if it was set
> - * via explicit fencing interfaces on the atomic ioctl. In that case it will
> - * drop the reference to the fence as we are not storing it anywhere.
> - * Otherwise, if &drm_plane_state.fence is not set this function we just set it
> - * with the received implicit fence. In both cases this function consumes a
> - * reference for @fence.
> - *
> - * This way explicit fencing can be used to overrule implicit fencing, which is
> - * important to make explicit fencing use-cases work: One example is using one
> - * buffer for 2 screens with different refresh rates. Implicit fencing will
> - * clamp rendering to the refresh rate of the slower screen, whereas explicit
> - * fence allows 2 independent render and display loops on a single buffer. If a
> - * driver allows obeys both implicit and explicit fences for plane updates, then
> - * it will break all the benefits of explicit fencing.
> - */
> -void
> -drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> -			       struct dma_fence *fence)
> -{
> -	if (plane_state->fence) {
> -		dma_fence_put(fence);
> -		return;
> -	}
> -
> -	plane_state->fence = fence;
> -}
> -EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
> -
>   /**
>    * drm_atomic_set_crtc_for_connector - set CRTC for connector
>    * @conn_state: atomic state object for the connector
> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> index a6d89aed0bda..8fc0b42acdff 100644
> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0-or-later
>   
>   #include <linux/dma-resv.h>
> +#include <linux/dma-fence-chain.h>
>   
>   #include <drm/drm_atomic_state_helper.h>
>   #include <drm/drm_atomic_uapi.h>
> @@ -141,25 +142,67 @@
>    * See drm_atomic_set_fence_for_plane() for a discussion of implicit and

This comment still refers to the function you just deleted. Maybe the 
deleted docs could be integrated here somehow, if still relevant?

>    * explicit fencing in atomic modeset updates.
>    */
> -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
> +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
> +				    struct drm_plane_state *state)

We have a 100-character limit. Please leave this on the same line.

>   {
> +	struct dma_fence *fence = dma_fence_get(state->fence);
>   	struct drm_gem_object *obj;

I'd declare this variable within the for loop.

> -	struct dma_fence *fence;
> +	enum dma_resv_usage usage;
> +	size_t i;
>   	int ret;
>   
>   	if (!state->fb)
>   		return 0;
>   
> -	obj = drm_gem_fb_get_obj(state->fb, 0);
> -	ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
> -	if (ret)
> -		return ret;
> -
> -	/* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
> -	 * to handle more fences in general for multiple BOs per fb.
> +	/*
> +	 * Only add the kernel fences here if there is already a fence set via
> +	 * explicit fencing interfaces on the atomic ioctl.
> +	 *
> +	 * This way explicit fencing can be used to overrule implicit fencing,
> +	 * which is important to make explicit fencing use-cases work: One
> +	 * example is using one buffer for 2 screens with different refresh
> +	 * rates. Implicit fencing will clamp rendering to the refresh rate of
> +	 * the slower screen, whereas explicit fence allows 2 independent
> +	 * render and display loops on a single buffer. If a driver allows
> +	 * obeys both implicit and explicit fences for plane updates, then it
> +	 * will break all the benefits of explicit fencing.
>   	 */
> -	drm_atomic_set_fence_for_plane(state, fence);
> +	usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
> +
> +	for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {

Instead of ARRAY_SIZE, rather use state->fb->format->num_planes. It's 
the number of planes (i.e., GEM objects) in the framebuffer.

> +		struct dma_fence *new;
> +
> +		obj = drm_gem_fb_get_obj(state->fb, i);
> +		if (!obj)

With the use of num_planes in the for loop, there should probably be a 
drm_WARN_ON_ONCE() around this test.

> +			continue;

goto error handling.

Or is there a use case for framebuffers with empty planes? At least it's 
not possible to instantiate one via drm_gem_fb_init_with_funcs().

> +
> +		ret = dma_resv_get_singleton(obj->resv, usage, &new);
> +		if (ret)
> +			goto error;
> +
> +		if (new && fence) {
> +			struct dma_fence_chain *chain = dma_fence_chain_alloc();
> +
> +			if (!chain) {
> +				ret = -ENOMEM;
> +				goto error;
> +			}
> +
> +			dma_fence_chain_init(chain, fence, new, 1);
> +			fence = &chain->base;
> +
> +		} else if (new) {
> +			fence = new;
> +		}
> +	}
> +
> +	dma_fence_put(state->fence);
> +	state->fence = fence;
>   	return 0;
> +
> +error:
> +	dma_fence_put(fence)
> +	return ret;
>   }
>   EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
>   
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 8cec52ad1277..4c6d39d7bdb2 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>   			      struct drm_crtc *crtc);
>   void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>   				 struct drm_framebuffer *fb);
> -void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> -				    struct dma_fence *fence);
>   int __must_check
>   drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>   				  struct drm_crtc *crtc);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 2628c7cde2da..89ea54652e87 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -74,9 +74,7 @@ struct drm_plane_state {
>   	 *
>   	 * Optional fence to wait for before scanning out @fb. The core atomic
>   	 * code will set this when userspace is using explicit fencing. Do not
> -	 * write this field directly for a driver's implicit fence, use
> -	 * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is
> -	 * preserved.
> +	 * write this field directly for a driver's implicit fence.
>   	 *
>   	 * Drivers should store any implicit fence in this from their
>   	 * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Christian König]

Am 28.04.22 um 09:23 schrieb Thomas Zimmermann:
> [SNIP]
>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c 
>> b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> index a6d89aed0bda..8fc0b42acdff 100644
>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>     #include <linux/dma-resv.h>
>> +#include <linux/dma-fence-chain.h>
>>     #include <drm/drm_atomic_state_helper.h>
>>   #include <drm/drm_atomic_uapi.h>
>> @@ -141,25 +142,67 @@
>>    * See drm_atomic_set_fence_for_plane() for a discussion of 
>> implicit and
>
> This comment still refers to the function you just deleted. Maybe the 
> deleted docs could be integrated here somehow, if still relevant?

Yeah, Daniel point that out as well.

>
>>    * explicit fencing in atomic modeset updates.
>>    */
>> -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct 
>> drm_plane_state *state)
>> +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
>> +                    struct drm_plane_state *state)
>
> We have a 100-character limit. Please leave this on the same line.

Despite some efforts to change this that is still documented as 
80-character limit: 
https://www.kernel.org/doc/html/v5.18-rc4/process/coding-style.html#breaking-long-lines-and-strings

>
>>   {
>> +    struct dma_fence *fence = dma_fence_get(state->fence);
>>       struct drm_gem_object *obj;
>
> I'd declare this variable within the for loop.
>
>> -    struct dma_fence *fence;
>> +    enum dma_resv_usage usage;
>> +    size_t i;
>>       int ret;
>>         if (!state->fb)
>>           return 0;
>>   -    obj = drm_gem_fb_get_obj(state->fb, 0);
>> -    ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, 
>> &fence);
>> -    if (ret)
>> -        return ret;
>> -
>> -    /* TODO: drm_atomic_set_fence_for_plane() should be changed to 
>> be able
>> -     * to handle more fences in general for multiple BOs per fb.
>> +    /*
>> +     * Only add the kernel fences here if there is already a fence 
>> set via
>> +     * explicit fencing interfaces on the atomic ioctl.
>> +     *
>> +     * This way explicit fencing can be used to overrule implicit 
>> fencing,
>> +     * which is important to make explicit fencing use-cases work: One
>> +     * example is using one buffer for 2 screens with different refresh
>> +     * rates. Implicit fencing will clamp rendering to the refresh 
>> rate of
>> +     * the slower screen, whereas explicit fence allows 2 independent
>> +     * render and display loops on a single buffer. If a driver allows
>> +     * obeys both implicit and explicit fences for plane updates, 
>> then it
>> +     * will break all the benefits of explicit fencing.
>>        */
>> -    drm_atomic_set_fence_for_plane(state, fence);
>> +    usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
>
> Instead of ARRAY_SIZE, rather use state->fb->format->num_planes. It's 
> the number of planes (i.e., GEM objects) in the framebuffer.
>
>> +        struct dma_fence *new;
>> +
>> +        obj = drm_gem_fb_get_obj(state->fb, i);
>> +        if (!obj)
>
> With the use of num_planes in the for loop, there should probably be a 
> drm_WARN_ON_ONCE() around this test.
>
>> +            continue;
>
> goto error handling.
>
> Or is there a use case for framebuffers with empty planes? At least 
> it's not possible to instantiate one via drm_gem_fb_init_with_funcs().
>

I was asking myself the same thing, but found this handling be used at 
other places as well.

Thanks for taking a look,
Christian.

Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4628 bytes --]

Hi

Am 28.04.22 um 09:32 schrieb Christian König:
> Am 28.04.22 um 09:23 schrieb Thomas Zimmermann:
>> [SNIP]
>>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c 
>>> b/drivers/gpu/drm/drm_gem_atomic_helper.c
>>> index a6d89aed0bda..8fc0b42acdff 100644
>>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>>> @@ -1,6 +1,7 @@
>>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>>     #include <linux/dma-resv.h>
>>> +#include <linux/dma-fence-chain.h>
>>>     #include <drm/drm_atomic_state_helper.h>
>>>   #include <drm/drm_atomic_uapi.h>
>>> @@ -141,25 +142,67 @@
>>>    * See drm_atomic_set_fence_for_plane() for a discussion of 
>>> implicit and
>>
>> This comment still refers to the function you just deleted. Maybe the 
>> deleted docs could be integrated here somehow, if still relevant?
> 
> Yeah, Daniel point that out as well.
> 
>>
>>>    * explicit fencing in atomic modeset updates.
>>>    */
>>> -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct 
>>> drm_plane_state *state)
>>> +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
>>> +                    struct drm_plane_state *state)
>>
>> We have a 100-character limit. Please leave this on the same line.
> 
> Despite some efforts to change this that is still documented as 
> 80-character limit: 
> https://www.kernel.org/doc/html/v5.18-rc4/process/coding-style.html#breaking-long-lines-and-strings 

But didn't checkpatch stop warning about the 80-char limit?

> 
> 
>>
>>>   {
>>> +    struct dma_fence *fence = dma_fence_get(state->fence);
>>>       struct drm_gem_object *obj;
>>
>> I'd declare this variable within the for loop.
>>
>>> -    struct dma_fence *fence;
>>> +    enum dma_resv_usage usage;
>>> +    size_t i;
>>>       int ret;
>>>         if (!state->fb)
>>>           return 0;
>>>   -    obj = drm_gem_fb_get_obj(state->fb, 0);
>>> -    ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, 
>>> &fence);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>> -    /* TODO: drm_atomic_set_fence_for_plane() should be changed to 
>>> be able
>>> -     * to handle more fences in general for multiple BOs per fb.
>>> +    /*
>>> +     * Only add the kernel fences here if there is already a fence 
>>> set via
>>> +     * explicit fencing interfaces on the atomic ioctl.
>>> +     *
>>> +     * This way explicit fencing can be used to overrule implicit 
>>> fencing,
>>> +     * which is important to make explicit fencing use-cases work: One
>>> +     * example is using one buffer for 2 screens with different refresh
>>> +     * rates. Implicit fencing will clamp rendering to the refresh 
>>> rate of
>>> +     * the slower screen, whereas explicit fence allows 2 independent
>>> +     * render and display loops on a single buffer. If a driver allows
>>> +     * obeys both implicit and explicit fences for plane updates, 
>>> then it
>>> +     * will break all the benefits of explicit fencing.
>>>        */
>>> -    drm_atomic_set_fence_for_plane(state, fence);
>>> +    usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
>>
>> Instead of ARRAY_SIZE, rather use state->fb->format->num_planes. It's 
>> the number of planes (i.e., GEM objects) in the framebuffer.
>>
>>> +        struct dma_fence *new;
>>> +
>>> +        obj = drm_gem_fb_get_obj(state->fb, i);
>>> +        if (!obj)
>>
>> With the use of num_planes in the for loop, there should probably be a 
>> drm_WARN_ON_ONCE() around this test.
>>
>>> +            continue;
>>
>> goto error handling.
>>
>> Or is there a use case for framebuffers with empty planes? At least 
>> it's not possible to instantiate one via drm_gem_fb_init_with_funcs().
>>
> 
> I was asking myself the same thing, but found this handling be used at 
> other places as well.

I've gradually changed the code towards the use of num_planes and 
stricter error reporting.  IMHO at some point, we should warn about 
empty plane BOs directly within drm_gem_fb_get_obj().

Best regards
Thomas

> 
> Thanks for taking a look,
> Christian.

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm: handle kernel fences in drm_gem_plane_helper_prepare_fb

[Daniel Vetter]

On Thu, Apr 28, 2022 at 08:41:28AM +0200, Christian König wrote:
> Am 27.04.22 um 18:03 schrieb Daniel Vetter:
> > On Thu, Apr 21, 2022 at 09:10:02PM +0200, Christian König wrote:
> > > drm_gem_plane_helper_prepare_fb() was using
> > > drm_atomic_set_fence_for_plane() which ignores all implicit fences when an
> > > explicit fence is already set. That's rather unfortunate when the fb still
> > > has a kernel fence we need to wait for to avoid presenting garbage on the
> > > screen.
> > > 
> > > So instead update the fence in the plane state directly. While at it also
> > > take care of all potential GEM objects and not just the first one.
> > > 
> > > Also remove the now unused drm_atomic_set_fence_for_plane() function, new
> > > drivers should probably use the atomic helpers directly.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > Is this enough to have amdgpu (and well others using ttm) fully rely on
> > this for atomic kms updates? Anything to clean up there? Would be neat to
> > include that in this series too if there's anything.
> 
> At least I strongly think so. I just haven't removed the
> dma_resv_wait_timeout() from amdgpu_dm_commit_planes() because that is
> simply not code I'm very familiar with.

Yeah defo not in the same patch, but a follow up would be really good.
Then DC folks could confirm that we got this right.

I think some other drivers would also benefit from some cleanup:

- nv50_wndw_prepare_fb could replace it's open-coded prepare with a call
  to the helper here I think.

- qxl looks buggy, at least I didn't find any wait for bo->moving, nor
  does the reserve seem to be synchronous qxl_bo_pin. Adding a call to the
  helper below in qxl_plane_prepare_fb() after the call to qxl_bo_pin() is
  probably the right thing.

- vwmgfx isn't gem, so can't use the gem helper outright (I think at
  least), but also looks buggy in the same way.

I'd include patches for amd/dc, nv and qxl and let the expert test it.
Just so we have some confirmation that this all now actually works,
without any driver hacks and stuff. The only really important thing is
that they're gem drivers, otherwise the bo pointers the helper relies on
aren't filled out in the drm_fb structure.
-Daniel

> 
> > > ---
> > >   drivers/gpu/drm/drm_atomic_uapi.c       | 37 ---------------
> > >   drivers/gpu/drm/drm_gem_atomic_helper.c | 63 +++++++++++++++++++++----
> > >   include/drm/drm_atomic_uapi.h           |  2 -
> > >   include/drm/drm_plane.h                 |  4 +-
> > >   4 files changed, 54 insertions(+), 52 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index c6394ba13b24..0f461261b3f3 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -254,43 +254,6 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> > >   }
> > >   EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
> > > -/**
> > > - * drm_atomic_set_fence_for_plane - set fence for plane
> > > - * @plane_state: atomic state object for the plane
> > > - * @fence: dma_fence to use for the plane
> > > - *
> > > - * Helper to setup the plane_state fence in case it is not set yet.
> > > - * By using this drivers doesn't need to worry if the user choose
> > > - * implicit or explicit fencing.
> > > - *
> > > - * This function will not set the fence to the state if it was set
> > > - * via explicit fencing interfaces on the atomic ioctl. In that case it will
> > > - * drop the reference to the fence as we are not storing it anywhere.
> > > - * Otherwise, if &drm_plane_state.fence is not set this function we just set it
> > > - * with the received implicit fence. In both cases this function consumes a
> > > - * reference for @fence.
> > > - *
> > > - * This way explicit fencing can be used to overrule implicit fencing, which is
> > > - * important to make explicit fencing use-cases work: One example is using one
> > > - * buffer for 2 screens with different refresh rates. Implicit fencing will
> > > - * clamp rendering to the refresh rate of the slower screen, whereas explicit
> > > - * fence allows 2 independent render and display loops on a single buffer. If a
> > > - * driver allows obeys both implicit and explicit fences for plane updates, then
> > > - * it will break all the benefits of explicit fencing.
> > > - */
> > > -void
> > > -drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> > > -			       struct dma_fence *fence)
> > I was a bit on the fence with ditching this, but the only offender not
> > using the prepare_fb helpers is i915, and so just more reasons that i915
> > needs to get off its hand-rolled atomic code with its own funky dependency
> > handling and everything.
> 
> Yeah, agree totally. amdgpu should now also work out of the box, I just
> didn't dared to actually enable it in the same patch.
> 
> > 
> > > -{
> > > -	if (plane_state->fence) {
> > > -		dma_fence_put(fence);
> > > -		return;
> > > -	}
> > > -
> > > -	plane_state->fence = fence;
> > > -}
> > > -EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
> > > -
> > >   /**
> > >    * drm_atomic_set_crtc_for_connector - set CRTC for connector
> > >    * @conn_state: atomic state object for the connector
> > > diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> > > index a6d89aed0bda..8fc0b42acdff 100644
> > > --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> > > @@ -1,6 +1,7 @@
> > >   // SPDX-License-Identifier: GPL-2.0-or-later
> > >   #include <linux/dma-resv.h>
> > > +#include <linux/dma-fence-chain.h>
> > >   #include <drm/drm_atomic_state_helper.h>
> > >   #include <drm/drm_atomic_uapi.h>
> > > @@ -141,25 +142,67 @@
> > >    * See drm_atomic_set_fence_for_plane() for a discussion of implicit and
> > You forgot to update the kerneldoc here, and also the reference to the
> > same function in the IN_FENCE_FD.
> > 
> > I think it'd be best to put a reference to that DOC: section here, and
> > adjust the uapi property doc to just state that the explicit fence will
> > overrule implicit sync. And then maybe also mention here that USAGE_KERNEL
> > fences are still obeyed.
> > 
> > With these changes (which should make sure that all references to
> > drm_atomic_set_fence_for_plane() are truly gone) this is
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Ok, going to update the doc and push this if nobody objects.
> 
> Thanks,
> Christian.
> 
> > 
> > 
> > 
> > >    * explicit fencing in atomic modeset updates.
> > >    */
> > > -int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state)
> > > +int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane,
> > > +				    struct drm_plane_state *state)
> > >   {
> > > +	struct dma_fence *fence = dma_fence_get(state->fence);
> > >   	struct drm_gem_object *obj;
> > > -	struct dma_fence *fence;
> > > +	enum dma_resv_usage usage;
> > > +	size_t i;
> > >   	int ret;
> > >   	if (!state->fb)
> > >   		return 0;
> > > -	obj = drm_gem_fb_get_obj(state->fb, 0);
> > > -	ret = dma_resv_get_singleton(obj->resv, DMA_RESV_USAGE_WRITE, &fence);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	/* TODO: drm_atomic_set_fence_for_plane() should be changed to be able
> > > -	 * to handle more fences in general for multiple BOs per fb.
> > > +	/*
> > > +	 * Only add the kernel fences here if there is already a fence set via
> > > +	 * explicit fencing interfaces on the atomic ioctl.
> > > +	 *
> > > +	 * This way explicit fencing can be used to overrule implicit fencing,
> > > +	 * which is important to make explicit fencing use-cases work: One
> > > +	 * example is using one buffer for 2 screens with different refresh
> > > +	 * rates. Implicit fencing will clamp rendering to the refresh rate of
> > > +	 * the slower screen, whereas explicit fence allows 2 independent
> > > +	 * render and display loops on a single buffer. If a driver allows
> > > +	 * obeys both implicit and explicit fences for plane updates, then it
> > > +	 * will break all the benefits of explicit fencing.
> > >   	 */
> > > -	drm_atomic_set_fence_for_plane(state, fence);
> > > +	usage = fence ? DMA_RESV_USAGE_KERNEL : DMA_RESV_USAGE_WRITE;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(state->fb->obj); ++i) {
> > > +		struct dma_fence *new;
> > > +
> > > +		obj = drm_gem_fb_get_obj(state->fb, i);
> > > +		if (!obj)
> > > +			continue;
> > > +
> > > +		ret = dma_resv_get_singleton(obj->resv, usage, &new);
> > > +		if (ret)
> > > +			goto error;
> > > +
> > > +		if (new && fence) {
> > > +			struct dma_fence_chain *chain = dma_fence_chain_alloc();
> > > +
> > > +			if (!chain) {
> > > +				ret = -ENOMEM;
> > > +				goto error;
> > > +			}
> > > +
> > > +			dma_fence_chain_init(chain, fence, new, 1);
> > > +			fence = &chain->base;
> > > +
> > > +		} else if (new) {
> > > +			fence = new;
> > > +		}
> > > +	}
> > > +
> > > +	dma_fence_put(state->fence);
> > > +	state->fence = fence;
> > >   	return 0;
> > > +
> > > +error:
> > > +	dma_fence_put(fence);
> > > +	return ret;
> > >   }
> > >   EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
> > > diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> > > index 8cec52ad1277..4c6d39d7bdb2 100644
> > > --- a/include/drm/drm_atomic_uapi.h
> > > +++ b/include/drm/drm_atomic_uapi.h
> > > @@ -49,8 +49,6 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> > >   			      struct drm_crtc *crtc);
> > >   void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> > >   				 struct drm_framebuffer *fb);
> > > -void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
> > > -				    struct dma_fence *fence);
> > >   int __must_check
> > >   drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> > >   				  struct drm_crtc *crtc);
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > index 2628c7cde2da..89ea54652e87 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -74,9 +74,7 @@ struct drm_plane_state {
> > >   	 *
> > >   	 * Optional fence to wait for before scanning out @fb. The core atomic
> > >   	 * code will set this when userspace is using explicit fencing. Do not
> > > -	 * write this field directly for a driver's implicit fence, use
> > > -	 * drm_atomic_set_fence_for_plane() to ensure that an explicit fence is
> > > -	 * preserved.
> > > +	 * write this field directly for a driver's implicit fence.
> > >   	 *
> > >   	 * Drivers should store any implicit fence in this from their
> > >   	 * &drm_plane_helper_funcs.prepare_fb callback. See drm_gem_plane_helper_prepare_fb()
> > > -- 
> > > 2.25.1
> > > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch