2016-04-26 Daniel Vetter daniel@ffwll.ch:
On Mon, Apr 25, 2016 at 07:33:28PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.padovan@collabora.co.uk
Support DRM out-fences creating a sync_file with a fence for each crtc update with the DRM_MODE_ATOMIC_OUT_FENCE flag.
We then send an struct drm_out_fences array with the out-fences fds back in the drm_atomic_ioctl() as an out arg in the out_fences_ptr field.
struct drm_out_fences { __u32 crtc_id; __u32 fd; };
v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
Comment by Daniel Vetter:
- Add clean up code for out_fences
Signed-off-by: Gustavo Padovan gustavo.padovan@collabora.co.uk
drivers/gpu/drm/drm_atomic.c | 163 +++++++++++++++++++++++++++++++++++++++++-- include/drm/drm_crtc.h | 10 +++ include/uapi/drm/drm_mode.h | 11 ++- 3 files changed, 179 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5f9d434..06c6007 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1566,6 +1566,133 @@ void drm_atomic_clean_old_fb(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_clean_old_fb);
+static struct drm_out_fence_state *get_out_fence(struct drm_device *dev,
struct drm_atomic_state *state,uint32_t __user *out_fences_ptr,uint64_t count_out_fences,uint64_t user_data)+{
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_out_fences *out_fences;
- struct drm_out_fence_state *fence_state;
- int num_fences = 0;
- int i, ret;
- if (count_out_fences > dev->mode_config.num_crtc)
return ERR_PTR(-EINVAL);- out_fences = kcalloc(count_out_fences, sizeof(*out_fences),
GFP_KERNEL);- if (!out_fences)
return ERR_PTR(-ENOMEM);A bit tricky, but the above kcalloc is the only thing that catches integer overflows in count_out_fences. Needs a comment imo since this could be a security exploit if we accidentally screw it up.
The check above makes sure that count_out_fences is not bigger than num_crtc. Don't that fix this?
Also needs a testcase imo.
- fence_state = kcalloc(count_out_fences, sizeof(*fence_state),
GFP_KERNEL);- if (!fence_state) {
kfree(out_fences);return ERR_PTR(-ENOMEM);- }
- for (i = 0 ; i < count_out_fences ; i++)
fence_state[i].fd = -1;- for_each_crtc_in_state(state, crtc, crtc_state, i) {
struct drm_pending_vblank_event *e;struct fence *fence;char name[32];fence = kzalloc(sizeof(*fence), GFP_KERNEL);if (!fence) {ret = -ENOMEM;goto out;}fence_init(fence, &drm_crtc_fence_ops, &crtc->fence_lock,crtc->fence_context, crtc->fence_seqno);snprintf(name, sizeof(name), "crtc-%d_%lu",drm_crtc_index(crtc), crtc->fence_seqno++);Hm ... fence_init_with_name? I'm kinda confused why we only name fences that are exported though, and why not all of them. Debugging fence deadlocks is real hard, so giving them all names might be a good idea.
Anyway, seems like more room for a bit more sync_file/struct fence merging.
We just removed name from sync_file_create() so snprintf() is not even necessary here anymore.
fence_state[i].fd = get_unused_fd_flags(O_CLOEXEC);if (fence_state[i].fd < 0) {fence_put(fence);ret = fence_state[i].fd;goto out;}fence_state[i].sync_file = sync_file_create(name, fence);if(!fence_state[i].sync_file) {fence_put(fence);ret = -ENOMEM;goto out;}if (crtc_state->event) {crtc_state->event->base.fence = fence;} else {This looks a bit funny - I'd change the create event logic to create an event either if we have the either drm event or out-fence flag set.
Ok.
e = create_vblank_event(dev, NULL, fence, user_data);if (!e) {ret = -ENOMEM;goto out;}crtc_state->event = e;}out_fences[num_fences].crtc_id = crtc->base.id;out_fences[num_fences].fd = fence_state[i].fd;num_fences++;- }
- if (copy_to_user(out_fences_ptr, out_fences,
num_fences * sizeof(*out_fences))) {ret = -EFAULT;goto out;- }
- kfree(out_fences);
- return fence_state;
+out:
- for (i = 0 ; i < count_out_fences ; i++) {
if (fence_state[i].sync_file)sync_file_put(fence_state[i].sync_file);if (fence_state[i].fd >= 0)put_unused_fd(fence_state[i].fd);- }
- kfree(fence_state);
- kfree(out_fences);
- return ERR_PTR(ret);
+}
+static void install_out_fence(uint64_t count_out_fences,
struct drm_out_fence_state *state)+{
- int i;
- for (i = 0 ; i < count_out_fences ; i++) {
if (state[i].sync_file)sync_file_install(state[i].sync_file, state[i].fd);Is sync_file_install anything more than fd_install? Imo a wrapper for just that function is overkill and just hides stuff. I'd nuke it (another sync_file patch though). In dma-buf we also don't wrap it, we only have a convenience wrapper for users who want to combine the get_unused_flags+fd_install in one go. And maybe even that is silly.
Ok, I unlazied and it's indeed just a silly wrapper. Please nuke it.
already fixed in the sync file de-stage patches.
- }
+}
+static void release_out_fence(uint64_t count_out_fences,
struct drm_out_fence_state *state)+{
- int i;
- for (i = 0 ; i < count_out_fences ; i++) {
if (state->sync_file)sync_file_put(state->sync_file);if (state->fd >= 0)put_unused_fd(state->fd);- }
+}
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1574,12 +1701,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr); uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr); uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
- uint32_t __user *out_fences_ptr = (uint32_t __user *)(unsigned long)(arg->out_fences_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;
- struct drm_out_fence_state *fence_state = NULL; unsigned plane_mask; int ret = 0; unsigned int i, j;
@@ -1605,9 +1734,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, !dev->mode_config.async_page_flip) return -EINVAL;
- if ((arg->flags & DRM_MODE_ATOMIC_OUT_FENCE) && !arg->count_out_fences)
return -EINVAL;We need testcases which check that arg->count_out_fences and arg->out_fences are 0 when the OUT_FENCE flag is not set.
Definitely needs an igt testcase for this invalid input case. Ofc we also need tests that give the kernel nonsens in count_out_fences and out_fences with the flag set.
- /* can't test and expect an event at the same time. */ if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
(arg->flags & (DRM_MODE_PAGE_FLIP_EVENT| DRM_MODE_ATOMIC_OUT_FENCE)))If you go with my suggestion above to create the event if either is set, maybe a DRM_MODE_ATOMIC_EVENT_MASK with both? Would read easier.
Ok.
Gustavo