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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 13, 2016 at 10:11:47AM +0200, Stefan Christ wrote:
> 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@phytec.de>
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c |  1 +
>  drivers/gpu/drm/drm_fb_helper.c     | 43 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_fb_helper.h         |  2 ++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index be66042..95657da 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -313,6 +313,7 @@ static struct fb_ops drm_fbdev_cma_ops = {
>  	.fb_blank	= drm_fb_helper_blank,
>  	.fb_pan_display	= drm_fb_helper_pan_display,
>  	.fb_setcmap	= drm_fb_helper_setcmap,
> +	.fb_ioctl       = drm_fb_helper_ioctl,
>  };
>  
>  static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7c2eb75..4d1f9b9 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1167,6 +1167,49 @@ 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:
> + * @cmd: ioctl like FBIO_WAITFORVSYNC
> + * @arg: ioctl argument

A bit more verbose kerneldoc would be great. Also, if you add this I think
we should roll it out for all drivers, for consistency of the supported
fbdev features when using emulation.
-Daniel

> + */
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	unsigned int i;
> +	int ret = 0;

You must first check whether fbdev owns the kms driver by calling
drm_fb_helper_is_bound(). Not sure whether the ioctl should fail, or
whether you instead should msleep(16) to throttle userspace a bit. Up to
you really.

> +
> +	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 active.
> +			 */
> +			if (!crtc->enabled)
> +				continue;

crtc->enabled != crtc->active, and drm_crtc_vblank_get only works when the
crtc is active. This means this ioctl will fail if the display is
suspended using dpms off/FB_BLANK. Is that what you want?

> +
> +			ret = drm_crtc_vblank_get(crtc);
> +			if (!ret) {
> +				drm_crtc_wait_one_vblank(crtc);
> +				drm_crtc_vblank_put(crtc);
> +			}

			else
				break;

Probably you also need some locking here, drm_modeset_lock(&crtc->mutex);
should be enough. Without that you might race with a dpms off. Might need
more locks, not entirely sure, perhaps also dev->mode_config.mutex for
fbdev emulation state.
-Daniel

> +		}
> +		return ret;
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_ioctl);
> +
> +/**
>   * drm_fb_helper_check_var - implementation for ->fb_check_var
>   * @var: screeninfo to check
>   * @info: fbdev registered by the helper
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 5b4aa35..121af93 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -278,6 +278,8 @@ void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, int state);
>  
>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
>  
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg);
> +
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel);
>  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper);
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel