Message-ID: <20170209170111.mmp7bpjy6qfl5kow@phenom.ffwll.local>
Date:   Thu, 9 Feb 2017 18:01:11 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:     Daniel Vetter <daniel.vetter@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        Stefan Christ <s.christ@...tec.de>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> From: Stefan Christ <s.christ@...tec.de>
> 
> 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@...tec.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 55 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     |  5 ++-
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e934b541feea..39a3532e311c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1234,6 +1234,61 @@ 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;
> +	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, and the interface spec for vblank_get says you'll get
an -EINVAL if the pipe is off. I'd say drop this, and instead eat the
-EINVAL from vblank_get and explain in a comment why you do that.

The trouble is that ->enabled is legacy state, atomic drivers don't need
to update it (helpers do it by default, but only for transition reasons),
and it's not synchronized to the real state changes at all. vblank_get
otoh is.

Otherwise lgtm.
-Daniel

> +
> +			ret = drm_crtc_vblank_get(crtc);
> +			if (!ret) {
> +				drm_crtc_wait_one_vblank(crtc);
> +				drm_crtc_vblank_put(crtc);
> +			}
> +		}
> +		goto unlock;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +unlock:
> +	drm_modeset_unlock_all(dev);
> +	return ret;
> +}
> +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 975deedd593e..460be9d9fa98 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -230,7 +230,8 @@ struct drm_fb_helper {
>  	.fb_blank	= drm_fb_helper_blank, \
>  	.fb_pan_display	= drm_fb_helper_pan_display, \
>  	.fb_debug_enter = drm_fb_helper_debug_enter, \
> -	.fb_debug_leave = drm_fb_helper_debug_leave
> +	.fb_debug_leave = drm_fb_helper_debug_leave, \
> +	.fb_ioctl	= drm_fb_helper_ioctl
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
> @@ -286,6 +287,8 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
>  
>  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);
> -- 
> git-series 0.8.11
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists