Re: [RFC 2/4] drm: writeback: Add out-fences for writeback connectors

[Sean Paul]

On Fri, Feb 23, 2018 at 08:17:52AM -0500, Rob Clark wrote:
> From: Brian Starkey 
> 
> Add the WRITEBACK_OUT_FENCE_PTR property to writeback connectors, to
> enable userspace to get a fence which will signal once the writeback is
> complete. It is not allowed to request an out-fence without a
> framebuffer attached to the connector.
> 
> A timeline is added to drm_writeback_connector for use by the writeback
> out-fences.
> 
> In the case of a commit failure or DRM_MODE_ATOMIC_TEST_ONLY, the fence
> is set to -1.
> 
> Changes from v2:
>  - Rebase onto Gustavo Padovan's v9 explicit sync series
>  - Change out_fence_ptr type to s32 __user *
>  - Set *out_fence_ptr to -1 in drm_atomic_connector_set_property
>  - Store fence in drm_writeback_job
>  Gustavo Padovan:
>  - Move out_fence_ptr out of connector_state
>  - Signal fence from drm_writeback_signal_completion instead of
>in driver directly
> 
> Changes from v3:
>  - Rebase onto 7e9081c5aac7 drm/fence: fix memory overwrite when setting 
> out_fence fd
>(change out_fence_ptr to s32 __user *, for real this time.)
>  - Update documentation around WRITEBACK_OUT_FENCE_PTR
> 
> Signed-off-by: Brian Starkey 
> [rebased and fixed conflicts]
> Signed-off-by: Mihail Atanassov 
> Signed-off-by: Liviu Dudau 
> Signed-off-by: Rob Clark 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_atomic.c|  99 
>  drivers/gpu/drm/drm_writeback.c | 109 
> +++-
>  include/drm/drm_atomic.h|   8 +++
>  include/drm/drm_connector.h |   8 +--
>  include/drm/drm_mode_config.h   |   8 +++
>  include/drm/drm_writeback.h |  41 ++-
>  6 files changed, 257 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 019f131fe8be..fc8c4da409ff 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -318,6 +318,35 @@ static s32 __user *get_out_fence_for_crtc(struct 
> drm_atomic_state *state,
>   return fence_ptr;
>  }
>  
> +static int set_out_fence_for_connector(struct drm_atomic_state *state,
> + struct drm_connector *connector,
> + s32 __user *fence_ptr)
> +{
> + unsigned int index = drm_connector_index(connector);
> +
> + if (!fence_ptr)
> + return 0;
> +
> + if (put_user(-1, fence_ptr))
> + return -EFAULT;
> +
> + state->connectors[index].out_fence_ptr = fence_ptr;
> +
> + return 0;
> +}
> +
> +static s32 __user *get_out_fence_for_connector(struct drm_atomic_state 
> *state,
> +struct drm_connector *connector)
> +{
> + unsigned int index = drm_connector_index(connector);
> + s32 __user *fence_ptr;
> +
> + fence_ptr = state->connectors[index].out_fence_ptr;
> + state->connectors[index].out_fence_ptr = NULL;
> +
> + return fence_ptr;
> +}
> +
>  /**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
> @@ -676,6 +705,12 @@ static int drm_atomic_connector_check(struct 
> drm_connector *connector,
>   return -EINVAL;
>   }
>  
> + if (writeback_job->out_fence && !writeback_job->fb) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence 
> without framebuffer\n",
> +  connector->base.id, connector->name);
> + return -EINVAL;
> + }
> +
>   return 0;
>  }
>  
> @@ -1277,6 +1312,11 @@ static int drm_atomic_connector_set_property(struct 
> drm_connector *connector,
>   if (fb)
>   drm_framebuffer_unreference(fb);
>   return ret;
> + } else if (property == config->writeback_out_fence_ptr_property) {
> + s32 __user *fence_ptr = u64_to_user_ptr(val);
> +
> + return set_out_fence_for_connector(state->state, connector,
> +fence_ptr);
>   } else if (connector->funcs->atomic_set_property) {
>   return connector->funcs->atomic_set_property(connector,
>   state, property, val);
> @@ -1361,6 +1401,8 @@ drm_atomic_connector_get_property(struct drm_connector 
> *connector,
>   } else if (property == config->writeback_fb_id_property) {
>   /* Writeback framebuffer is one-shot, write and forget */
>   *val = 0;
> + } else if (property == config->writeback_out_fence_ptr_property) {
> + *val = 0;
>   } else if (connector->funcs->atomic_get_property) {
>   return connector->funcs->atomic_get_property(connector,
>   state, property, val);
> @@ -2221,7 +2263,7 @@ static int setup_out_fence(struct 

Re: [RFC 2/4] drm: writeback: Add out-fences for writeback connectors

[Sean Paul]

On Fri, Feb 23, 2018 at 08:17:52AM -0500, Rob Clark wrote:
> From: Brian Starkey 
> 
> Add the WRITEBACK_OUT_FENCE_PTR property to writeback connectors, to
> enable userspace to get a fence which will signal once the writeback is
> complete. It is not allowed to request an out-fence without a
> framebuffer attached to the connector.
> 
> A timeline is added to drm_writeback_connector for use by the writeback
> out-fences.
> 
> In the case of a commit failure or DRM_MODE_ATOMIC_TEST_ONLY, the fence
> is set to -1.
> 
> Changes from v2:
>  - Rebase onto Gustavo Padovan's v9 explicit sync series
>  - Change out_fence_ptr type to s32 __user *
>  - Set *out_fence_ptr to -1 in drm_atomic_connector_set_property
>  - Store fence in drm_writeback_job
>  Gustavo Padovan:
>  - Move out_fence_ptr out of connector_state
>  - Signal fence from drm_writeback_signal_completion instead of
>in driver directly
> 
> Changes from v3:
>  - Rebase onto 7e9081c5aac7 drm/fence: fix memory overwrite when setting 
> out_fence fd
>(change out_fence_ptr to s32 __user *, for real this time.)
>  - Update documentation around WRITEBACK_OUT_FENCE_PTR
> 
> Signed-off-by: Brian Starkey 
> [rebased and fixed conflicts]
> Signed-off-by: Mihail Atanassov 
> Signed-off-by: Liviu Dudau 
> Signed-off-by: Rob Clark 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_atomic.c|  99 
>  drivers/gpu/drm/drm_writeback.c | 109 
> +++-
>  include/drm/drm_atomic.h|   8 +++
>  include/drm/drm_connector.h |   8 +--
>  include/drm/drm_mode_config.h   |   8 +++
>  include/drm/drm_writeback.h |  41 ++-
>  6 files changed, 257 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 019f131fe8be..fc8c4da409ff 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -318,6 +318,35 @@ static s32 __user *get_out_fence_for_crtc(struct 
> drm_atomic_state *state,
>   return fence_ptr;
>  }
>  
> +static int set_out_fence_for_connector(struct drm_atomic_state *state,
> + struct drm_connector *connector,
> + s32 __user *fence_ptr)
> +{
> + unsigned int index = drm_connector_index(connector);
> +
> + if (!fence_ptr)
> + return 0;
> +
> + if (put_user(-1, fence_ptr))
> + return -EFAULT;
> +
> + state->connectors[index].out_fence_ptr = fence_ptr;
> +
> + return 0;
> +}
> +
> +static s32 __user *get_out_fence_for_connector(struct drm_atomic_state 
> *state,
> +struct drm_connector *connector)
> +{
> + unsigned int index = drm_connector_index(connector);
> + s32 __user *fence_ptr;
> +
> + fence_ptr = state->connectors[index].out_fence_ptr;
> + state->connectors[index].out_fence_ptr = NULL;
> +
> + return fence_ptr;
> +}
> +
>  /**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
> @@ -676,6 +705,12 @@ static int drm_atomic_connector_check(struct 
> drm_connector *connector,
>   return -EINVAL;
>   }
>  
> + if (writeback_job->out_fence && !writeback_job->fb) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence 
> without framebuffer\n",
> +  connector->base.id, connector->name);
> + return -EINVAL;
> + }
> +
>   return 0;
>  }
>  
> @@ -1277,6 +1312,11 @@ static int drm_atomic_connector_set_property(struct 
> drm_connector *connector,
>   if (fb)
>   drm_framebuffer_unreference(fb);
>   return ret;
> + } else if (property == config->writeback_out_fence_ptr_property) {
> + s32 __user *fence_ptr = u64_to_user_ptr(val);
> +
> + return set_out_fence_for_connector(state->state, connector,
> +fence_ptr);
>   } else if (connector->funcs->atomic_set_property) {
>   return connector->funcs->atomic_set_property(connector,
>   state, property, val);
> @@ -1361,6 +1401,8 @@ drm_atomic_connector_get_property(struct drm_connector 
> *connector,
>   } else if (property == config->writeback_fb_id_property) {
>   /* Writeback framebuffer is one-shot, write and forget */
>   *val = 0;
> + } else if (property == config->writeback_out_fence_ptr_property) {
> + *val = 0;
>   } else if (connector->funcs->atomic_get_property) {
>   return connector->funcs->atomic_get_property(connector,
>   state, property, val);
> @@ -2221,7 +2263,7 @@ static int setup_out_fence(struct drm_out_fence_state 
> *fence_state,
>   return 0;
>  }
>  
> -static int prepare_crtc_signaling(struct drm_device *dev,
> +static int