Re: [PATCH v3 2/2] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

[Stefan Lengfeld]

Hi Maxime,

On Wed, Feb 15, 2017 at 05:19:09PM +0100, Maxime Ripard wrote:
> From: Stefan Christ <s.christ@xxxxxxxxx>
> 

Maybe change the author here. Only the boilerplate code looks my original
patch. The real code is your work ;-)

> Implement legacy framebuffer ioctl FBIO_WAITFORVSYNC in the generic
> framebuffer emulation driver. Legacy framebuffer users like non kms/drm
> based OpenGL(ES)/EGL implementations may require the ioctl to
> synchronize drawing or buffer flip for double buffering. It is tested on
> the i.MX6.
> 
> Code is based on
>     https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xilinx/xilinx_drm_fb.c#L196
> 
> Signed-off-by: Stefan Christ <s.christ@xxxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 63 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     | 12 +++++-
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index c6de87abaca8..15ee9641c725 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1240,6 +1240,69 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
>  
>  /**
> + * drm_fb_helper_ioctl - legacy ioctl implementation
> + * @info: fbdev registered by the helper
> + * @cmd: ioctl command
> + * @arg: ioctl argument
> + *
> + * A helper to implement the standard fbdev ioctl. Only
> + * FBIO_WAITFORVSYNC is implemented for now.
> + */
> +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;
> +	struct drm_mode_set *mode_set;
> +	struct drm_crtc *crtc;
> +	u32 karg;
> +	int ret = 0;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	if (!drm_fb_helper_is_bound(fb_helper)) {
> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	switch (cmd) {
> +	case FBIO_WAITFORVSYNC:
> +		if (get_user(karg, (__u32 __user *)arg)) {
> +			ret = -EFAULT;
> +			goto unlock;
> +		}
> +
> +		if (karg >= fb_helper->crtc_count) {
> +			ret = -EINVAL;
> +			goto unlock;
> +		}

Ville SyrjÃlà said [1]:

    FBIO_WAITFORVSYNC takes the crtc as a parmeter, so I'm not sure we want
    to do this for all the crtcs. Though what that crtc means for fb is
    rather poorly defined.

Don't think it takes the crtc as a parameter in 'arg'. When you look at the
existing fb_ioctl implementations in the directory drivers/video/fbdev/, the
argument 'arg' is either ignored or must be '0'.

Furthmore most exiting userspace code just passes the value "0" as the
argument. For example DirectFB:

     static const int zero = 0;
     [...]
     if (ioctl( dfb_fbdev->fd, FBIO_WAITFORVSYNC, &zero ))

I see two options here: 

1/ Just wait for the first vsync event on the first enabled crtc. This is
   a good approximation for the old framebuffer API. It has no concept of
   multiple crtcs with concurrenctly running scan out processes. (Maybe apart
   from extra vendor implementations). So if there are really more than one active
   crtcs in the system, bad luck with framebuffer API. There will be tearing.
   Userspace has to use the new DRM/KMS API.

2/ Wait for a single vsync event on all active crtcs as Ville SyrjÃlà 
   described here [2] in the optimal implementation.

Kind regards / Mit freundlichen GrÃÃen
	Stefan Lengfeld

[1] https://lists.freedesktop.org/archives/dri-devel/2017-February/132617.html
[2] https://lists.freedesktop.org/archives/dri-devel/2017-February/132820.html