Re: [PATCH v8 2/3] drm: writeback: Add out-fences for writeback connectors
On Mon, May 21, 2018 at 12:02:25PM -0700, Eric Anholt wrote:
> Liviu Dudau writes:
>
> > 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.
>
>
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index cf3a28676006a..6a7462c1821ad 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -49,6 +49,32 @@ struct drm_writeback_connector {
> > * drm_writeback_signal_completion()
> > */
> > struct list_head job_queue;
> > +
> > + /**
> > +* @fence_context:
> > +*
> > +* timeline context used for fence operations.
> > +*/
> > + unsigned int fence_context;
> > + /**
> > +* @fence_lock:
> > +*
> > +* spinlock to protect the fences in the fence_context.
> > +*/
> > + spinlock_t fence_lock;
> > + /**
> > +* @fence_seqno:
> > +*
> > +* Seqno variable used as monotonic counter for the fences
> > +* created on the connector's timeline.
> > +*/
> > + unsigned long fence_seqno;
> > + /**
> > +* @timeline_name:
> > +*
> > +* The name of the connector's fence timeline.
> > +*/
> > + char timeline_name[32];
> > };
> >
> > struct drm_writeback_job {
> > @@ -59,12 +85,14 @@ struct drm_writeback_job {
> > * framebuffer reference to a workqueue.
> > */
> > struct work_struct cleanup_work;
> > +
> > /**
> > * @list_entry:
> > *
> > * List item for the connector's @job_queue
> > */
> > struct list_head list_entry;
> > +
> > /**
> > * @fb:
> > *
>
> Move this hunk into patch 1?
I can, however this is the only change I will be making. Is it worth
respinning a new revision for it?
>
> Other than that, the series is:
>
> Reviewed-by: Eric Anholt
Many thanks for that!
>
> It's pretty clean and makes sense to me. I only had some questions
> about the job_queue, which seems superfluous if we aren't supporting
> firing off a new writeback while an old one is outstanding (and maybe we
> should throw an error in that case). Still, I think this is ready to land.
I know Sean and Daniel are on holiday. Are you OK to pull this into
drm-misc? Should I ask Gustavo to do it?
Best regards,
Liviu
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
Re: [PATCH v8 2/3] drm: writeback: Add out-fences for writeback connectors
Liviu Dudau writes:
> 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.
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index cf3a28676006a..6a7462c1821ad 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -49,6 +49,32 @@ struct drm_writeback_connector {
>* drm_writeback_signal_completion()
>*/
> struct list_head job_queue;
> +
> + /**
> + * @fence_context:
> + *
> + * timeline context used for fence operations.
> + */
> + unsigned int fence_context;
> + /**
> + * @fence_lock:
> + *
> + * spinlock to protect the fences in the fence_context.
> + */
> + spinlock_t fence_lock;
> + /**
> + * @fence_seqno:
> + *
> + * Seqno variable used as monotonic counter for the fences
> + * created on the connector's timeline.
> + */
> + unsigned long fence_seqno;
> + /**
> + * @timeline_name:
> + *
> + * The name of the connector's fence timeline.
> + */
> + char timeline_name[32];
> };
>
> struct drm_writeback_job {
> @@ -59,12 +85,14 @@ struct drm_writeback_job {
>* framebuffer reference to a workqueue.
>*/
> struct work_struct cleanup_work;
> +
> /**
>* @list_entry:
>*
>* List item for the connector's @job_queue
>*/
> struct list_head list_entry;
> +
> /**
>* @fb:
>*
Move this hunk into patch 1?
Other than that, the series is:
Reviewed-by: Eric Anholt
It's pretty clean and makes sense to me. I only had some questions
about the job_queue, which seems superfluous if we aren't supporting
firing off a new writeback while an old one is outstanding (and maybe we
should throw an error in that case). Still, I think this is ready to land.
signature.asc
Description: PGP signature
[PATCH v8 2/3] drm: writeback: Add out-fences for writeback connectors
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 commit 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
---
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 | 43 -
6 files changed, 259 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3f1e4b894803b..cc2f86c68b293 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
@@ -705,6 +734,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;
}
@@ -1320,6 +1355,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);
@@ -1404,6 +1444,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);
@@ -2263,7 +2305,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 prepare_signaling(struct drm_device *dev,
struct drm_atomic_state *state,
struct drm_mode_atomic *arg,
struct drm_file *file_priv,
@@ -2272,6 +2314,8 @@ static int prepare_crtc_signaling(struct drm_devi
