Re: [PATCH v6 4/6] drm/fence: add in-fences support

[Daniel Vetter]

On Thu, Oct 27, 2016 at 05:37:09PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> 
> There is now a new property called FENCE_FD attached to every plane
> state that receives the sync_file fd from userspace via the atomic commit
> IOCTL.
> 
> The fd is then translated to a fence (that may be a fence_collection
> subclass or just a normal fence) and then used by DRM to fence_wait() for
> all fences in the sync_file to signal. So it only commits when all
> framebuffers are ready to scanout.
> 
> v2: Comments by Daniel Vetter:
> 	- remove set state->fence = NULL in destroy phase
> 	- accept fence -1 as valid and just return 0
> 	- do not call fence_get() - sync_file_fences_get() already calls it
> 	- fence_put() if state->fence is already set, in case userspace
> 	set the property more than once.
> 
> v3: WARN_ON if fence is set but state has no FB
> 
> v4: Comment from Maarten Lankhorst
> 	- allow set fence with no related fb
> 
> v5: rename FENCE_FD to IN_FENCE_FD
> 
> v6: Comments by Daniel Vetter:
> 	- rename plane_state->in_fence back to "fence"
> 
>      - rebase after fence -> dma_fence rename
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>

Looks good, but I'll wait with full review with all the fence patches
until the igts show up. It's much easier to check for gabs in input
validation code if you also have the testcases at hand. A few comments
below.
-Daniel

> ---
>  drivers/gpu/drm/Kconfig             |  1 +
>  drivers/gpu/drm/drm_atomic.c        | 15 +++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |  5 +++--
>  drivers/gpu/drm/drm_crtc.c          |  6 ++++++
>  drivers/gpu/drm/drm_plane.c         |  1 +
>  include/drm/drm_crtc.h              |  5 +++++
>  6 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 483059a..43cb33d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -12,6 +12,7 @@ menuconfig DRM
>  	select I2C
>  	select I2C_ALGOBIT
>  	select DMA_SHARED_BUFFER
> +	select SYNC_FILE
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5e73954..28d9366 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_plane_helper.h>
> +#include <linux/sync_file.h>
>  
>  #include "drm_crtc_internal.h"
>  
> @@ -686,6 +687,17 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		drm_atomic_set_fb_for_plane(state, fb);
>  		if (fb)
>  			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_in_fence_fd) {
> +		if (U642I64(val) == -1)
> +			return 0;
> +
> +		if (state->fence)
> +			dma_fence_put(state->fence);
> +
> +		state->fence = sync_file_get_fence(val);
> +		if (!state->fence)
> +			return -EINVAL;
> +
>  	} else if (property == config->prop_crtc_id) {
>  		struct drm_crtc *crtc = drm_crtc_find(dev, val);
>  		return drm_atomic_set_crtc_for_plane(state, crtc);
> @@ -747,6 +759,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	if (property == config->prop_fb_id) {
>  		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_in_fence_fd) {
> +		*val = -1;
>  	} else if (property == config->prop_crtc_id) {
>  		*val = (state->crtc) ? state->crtc->base.id : 0;
>  	} else if (property == config->prop_crtc_x) {
> @@ -1752,6 +1766,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		drm_mode_object_unreference(obj);
>  	}
>  
> +

Spurios whitespace.

>  	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  			struct drm_pending_vblank_event *e;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 75ad01d..c34da9e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1034,8 +1034,6 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
>  		if (!plane_state->fence)
>  			continue;
>  
> -		WARN_ON(!plane_state->fb);
> -

Why this? We don't allow a plane to be enabled without an fb, and adding a
fence to a plane which is disabled sounds likea  bug. We probably should
have a bit of code in the core atomic check code to make sure userspace
never asks for a fence when the plane is off.

>  		/*
>  		 * If waiting for fences pre-swap (ie: nonblock), userspace can
>  		 * still interrupt the operation. Instead of blocking until the
> @@ -3114,6 +3112,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
>  	if (state->fb)
>  		drm_framebuffer_unreference(state->fb);
> +
> +	if (state->fence)
> +		dma_fence_put(state->fence);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 13441e2..7878bfd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -397,6 +397,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_fb_id = prop;
>  
> +	prop = drm_property_create_signed_range(dev, DRM_MODE_PROP_ATOMIC,
> +			"IN_FENCE_FD", -1, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_in_fence_fd = prop;
> +
>  	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>  			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
>  	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..3957ef8 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -137,6 +137,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_in_fence_fd, -1);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
>  		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index fa1aa21..719b6a8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1201,6 +1201,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *prop_fb_id;
>  	/**
> +	 * @prop_in_fence_fd: Sync File fd representing the incoming fences
> +	 * for a Plane.
> +	 */
> +	struct drm_property *prop_in_fence_fd;
> +	/**
>  	 * @prop_crtc_id: Default atomic plane property to specify the
>  	 * &drm_crtc.
>  	 */
> -- 
> 2.5.5
> 

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