Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 09, 2017 at 05:38:26PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 9 February 2017 at 17:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> >> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> >> +{
> >> +     struct drm_fb_helper *fb_helper = info->par;
> >> +     struct drm_device *dev = fb_helper->dev;
> >> +     unsigned int i;
> >> +     int ret = 0;
> >> +
> >> +     drm_modeset_lock_all(dev);
> >> +     if (!drm_fb_helper_is_bound(fb_helper)) {
> >> +             drm_modeset_unlock_all(dev);
> >> +             return -EBUSY;
> >> +     }
> >> +
> >> +     switch (cmd) {
> >> +     case FBIO_WAITFORVSYNC:
> >> +             for (i = 0; i < fb_helper->crtc_count; i++) {
> >> +                     struct drm_mode_set *mode_set;
> >> +                     struct drm_crtc *crtc;
> >> +
> >> +                     mode_set = &fb_helper->crtc_info[i].mode_set;
> >> +                     crtc = mode_set->crtc;
> >> +
> >> +                     /*
> >> +                      * Only call drm_crtc_wait_one_vblank for crtcs that
> >> +                      * are currently enabled.
> >> +                      */
> >> +                     if (!crtc->enabled)
> >> +                             continue;
> >
> > This needs locking
> 
> More locking than the drm_modeset_lock_all() above the switch? Ouch. :)

Oh, I didn't spot that one. Well, that one should be removed imo, we dont
want any vblank wait path to hold these locks, it looks bad. Otoh we still
need to grab the dev->mode_config.mutex, because that's also the fbdev
lock. There's a plan somewhere to give the fbdev helpers their own lock,
so switching to just mode_config.mutex + the changes I suggestd would be
good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch