2016-04-26 Daniel Vetter <dan...@ffwll.ch>:

> On Mon, Apr 25, 2016 at 07:33:28PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.pado...@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.pado...@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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to