[PATCH v4 0/1] drm: Support framebuffer panning

[PATCH v4 0/1] drm: Support framebuffer panning

[Maxime Ripard]

Hi,

This is a respin of the previous serie called "Support fast framebuffer
panning for i.MX6" made by Stefan 6 monthes ago. The imx6 bits have been
removed, and the comments that were made at that time fixed (hopefully).

Let me know what you think,
Maxime

Changes from v3:
  - Dropped the overallocation patch that has been applied
  - Change authorship of the second patch as suggested by Stefan, the
    original author
  - Only consider the first CRTC and ignore the ioctl argument

Changes from v2:
  - Renamed the CONFIG_DRM_CMA_FBDEV_BUFFER_NUM to
    CONFIG_DRM_FBDEV_OVERALLOC, and changed it for a percentage
  - Moved the overallocation code into the core fbdev helpers to not rely
    on CMA anymore
  - Changed the locks taken by drm_fb_helper_ioctl to the mode_config mutex
    intead of calling drm_modeset_lock_all
  - Don't test against drm_crtc's enabled anymore, but rely on the error
    code of drm_crtc_vblank_get instead
  - Defined drm_fb_helper_ioctl when DRM_FBDEV_EMULATION is not set in
    order to fix a compilation error
  - Don't wait for all CRTC's vblank but only the one given in the ioctl
    argument

Changes from v1:
  - Added drm_fb_helper_ioctl to DRM_FB_HELPER_DEFAULT_OPS
  - Expanded a bit the kerneldoc for drm_fb_helper_ioctl
  - Added some locking to drm_fb_helper_ioctl
  - Checked that the framebuffer is indeed attached before allowing ioctl
  - Added a module parameter to specify the number of framebuffers to
    allocate

Initial cover letter: Support fast framebuffer panning for i.MX6

im currently working on supporting double/tripple buffering for the
framebuffer emulation on the i.MX6.  While working on it I noticed that the
mainline kernel does not support some features in the generic drm
framebuffer emulation for framebuffer panning and vsync synchronisation.
They are needed for simple framebuffer applications and some OpenGL
libraries using double buffering with FBIOPUT_VSCREENINFO,
FBIO_WAITFORVSYNC and FBIOPAN_DISPLAY.

Maxime Ripard (1):
  drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

 drivers/gpu/drm/drm_fb_helper.c | 68 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     | 12 +++++-
 2 files changed, 79 insertions(+), 1 deletion(-)

base-commit: 075ffb50cc16ab055b5d47b30163401bb356ab51
-- 
git-series 0.8.11

[PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

[Maxime Ripard]

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.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 68 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     | 12 +++++-
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index c6de87abaca8..8ac8716c023a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1240,6 +1240,74 @@ 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;
+	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:
+		/*
+		 * Only consider the first CRTC.
+		 *
+		 * This ioctl is supposed to take the CRTC number as
+		 * an argument, but in fbdev times, what that number
+		 * was supposed to be was quite unclear, different
+		 * drivers were passing that argument differently
+		 * (some by reference, some by value), and most of the
+		 * userspace applications were just hardcoding 0 as an
+		 * argument.
+		 *
+		 * The first CRTC should be the integrated panel on
+		 * most drivers, so this is the best choice we can
+		 * make. If we're not smart enough here, one should
+		 * just consider switch the userspace to KMS.
+		 */
+		mode_set = &fb_helper->crtc_info[0].mode_set;
+		crtc = mode_set->crtc;
+
+		/*
+		 * Only wait for a vblank event if the CRTC is
+		 * enabled, otherwise just don't do anythintg,
+		 * not even report an error.
+		 */
+		ret = drm_crtc_vblank_get(crtc);
+		if (!ret) {
+			drm_crtc_wait_one_vblank(crtc);
+			drm_crtc_vblank_put(crtc);
+		}
+
+		ret = 0;
+		goto unlock;
+	default:
+		ret = -ENOTTY;
+	}
+
+unlock:
+	mutex_unlock(&dev->mode_config.mutex);
+	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..2891888c6e41 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,9 @@ 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);
@@ -377,6 +381,12 @@ static inline int drm_fb_helper_setcmap(struct fb_cmap *cmap,
 	return 0;
 }
 
+static inline int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
+				      unsigned long arg)
+{
+	return 0;
+}
+
 static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
 {
 }
-- 
git-series 0.8.11

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

[Stefan Lengfeld]

Hi Maxime,

On Tue, Feb 28, 2017 at 04:36:51PM +0100, Maxime Ripard 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.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>

I will test this patch and the already applied patch to drm-next on the
i.MX6 platform in the next days.

Kind regards,
    Stefan 

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

[Daniel Vetter]

On Tue, Feb 28, 2017 at 04:36:51PM +0100, Maxime Ripard 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>

Pushed to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 68 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     | 12 +++++-
>  2 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index c6de87abaca8..8ac8716c023a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1240,6 +1240,74 @@ 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;
> +	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:
> +		/*
> +		 * Only consider the first CRTC.
> +		 *
> +		 * This ioctl is supposed to take the CRTC number as
> +		 * an argument, but in fbdev times, what that number
> +		 * was supposed to be was quite unclear, different
> +		 * drivers were passing that argument differently
> +		 * (some by reference, some by value), and most of the
> +		 * userspace applications were just hardcoding 0 as an
> +		 * argument.
> +		 *
> +		 * The first CRTC should be the integrated panel on
> +		 * most drivers, so this is the best choice we can
> +		 * make. If we're not smart enough here, one should
> +		 * just consider switch the userspace to KMS.
> +		 */
> +		mode_set = &fb_helper->crtc_info[0].mode_set;
> +		crtc = mode_set->crtc;
> +
> +		/*
> +		 * Only wait for a vblank event if the CRTC is
> +		 * enabled, otherwise just don't do anythintg,
> +		 * not even report an error.
> +		 */
> +		ret = drm_crtc_vblank_get(crtc);
> +		if (!ret) {
> +			drm_crtc_wait_one_vblank(crtc);
> +			drm_crtc_vblank_put(crtc);
> +		}
> +
> +		ret = 0;
> +		goto unlock;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +unlock:
> +	mutex_unlock(&dev->mode_config.mutex);
> +	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..2891888c6e41 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,9 @@ 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);
> @@ -377,6 +381,12 @@ static inline int drm_fb_helper_setcmap(struct fb_cmap *cmap,
>  	return 0;
>  }
>  
> +static inline int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
> +				      unsigned long arg)
> +{
> +	return 0;
> +}
> +
>  static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
>  {
>  }
> -- 
> git-series 0.8.11
> _______________________________________________
> 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

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

[Daniel Vetter]

On Thu, Mar 02, 2017 at 08:12:02AM +0100, Daniel Vetter wrote:
> On Tue, Feb 28, 2017 at 04:36:51PM +0100, Maxime Ripard 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.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Pushed to drm-misc, thanks.

I forgot to add Dave's irc ack to the commit patch, but for the record I
discussed it with him and he's not enthuastic, but ok.
-Daniel

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 68 ++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_fb_helper.h     | 12 +++++-
> >  2 files changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index c6de87abaca8..8ac8716c023a 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1240,6 +1240,74 @@ 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;
> > +	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:
> > +		/*
> > +		 * Only consider the first CRTC.
> > +		 *
> > +		 * This ioctl is supposed to take the CRTC number as
> > +		 * an argument, but in fbdev times, what that number
> > +		 * was supposed to be was quite unclear, different
> > +		 * drivers were passing that argument differently
> > +		 * (some by reference, some by value), and most of the
> > +		 * userspace applications were just hardcoding 0 as an
> > +		 * argument.
> > +		 *
> > +		 * The first CRTC should be the integrated panel on
> > +		 * most drivers, so this is the best choice we can
> > +		 * make. If we're not smart enough here, one should
> > +		 * just consider switch the userspace to KMS.
> > +		 */
> > +		mode_set = &fb_helper->crtc_info[0].mode_set;
> > +		crtc = mode_set->crtc;
> > +
> > +		/*
> > +		 * Only wait for a vblank event if the CRTC is
> > +		 * enabled, otherwise just don't do anythintg,
> > +		 * not even report an error.
> > +		 */
> > +		ret = drm_crtc_vblank_get(crtc);
> > +		if (!ret) {
> > +			drm_crtc_wait_one_vblank(crtc);
> > +			drm_crtc_vblank_put(crtc);
> > +		}
> > +
> > +		ret = 0;
> > +		goto unlock;
> > +	default:
> > +		ret = -ENOTTY;
> > +	}
> > +
> > +unlock:
> > +	mutex_unlock(&dev->mode_config.mutex);
> > +	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..2891888c6e41 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,9 @@ 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);
> > @@ -377,6 +381,12 @@ static inline int drm_fb_helper_setcmap(struct fb_cmap *cmap,
> >  	return 0;
> >  }
> >  
> > +static inline int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
> > +				      unsigned long arg)
> > +{
> > +	return 0;
> > +}
> > +
> >  static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
> >  {
> >  }
> > -- 
> > git-series 0.8.11
> > _______________________________________________
> > 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

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

Re: [PATCH v4 1/1] drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

[Stefan Lengfeld]

Hi Maxime,

On Tue, Feb 28, 2017 at 04:36:51PM +0100, Maxime Ripard 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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 68 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     | 12 +++++-
>  2 files changed, 79 insertions(+), 1 deletion(-)

I have tested the current HEAD of drm-misc-next. It includes this patch
and the patch ("drm/fb-helper: Add multi buffer support for cma fbdev").
On my i.MX6 board with HDMI output both patches work as advertised.
Multi-buffering with ioctls FBIO_WAITFORVSYNC and FBIOPAN_DISPLAY is
possible.

Kind Regards
    Stefan Lengfeld