[PATCH 0/3] Support fast framebuffer panning for i.MX6

[PATCH 0/3] Support fast framebuffer panning for i.MX6

[Stefan Christ]

Hi,

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.

Any comments?

Kind regards,
	Stefan Christ

Stefan Christ (2):
  drm: fb_helper: implement ioctl FBIO_WAITFORVSYNC
  drm/imx: ipuv3-crtc: implement fast path mode_set_base

Xinliang Liu (1):
  drm/cma-helper: Add multi buffer support for cma fbdev

 drivers/gpu/drm/Kconfig             |  8 +++++++
 drivers/gpu/drm/drm_fb_cma_helper.c |  9 +++++++-
 drivers/gpu/drm/drm_fb_helper.c     | 43 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/ipuv3-crtc.c    | 10 +++++++++
 include/drm/drm_fb_helper.h         |  2 ++
 5 files changed, 71 insertions(+), 1 deletion(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/cma-helper: Add multi buffer support for cma fbdev

[Stefan Christ]

From: Xinliang Liu <xinliang.liu@linaro.org>

This patch add a config to support to create multi buffer for cma fbdev.
Such as double buffer and triple buffer.

Cma fbdev is convient to add a legency fbdev. And still many Android
devices use fbdev now and at least double buffer is needed for these
Android devices, so that a buffer flip can be operated. It will need
some time for Android device vendors to abondon legency fbdev. So multi
buffer for fbdev is needed.

Signed-off-by: Xinliang Liu <xinliang.liu@linaro.org>
[s.christ@phytec.de: Picking patch from
                     https://lkml.org/lkml/2015/9/14/188]
Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/gpu/drm/Kconfig             | 8 ++++++++
 drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index fc35731..56cf34d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -106,6 +106,14 @@ config DRM_KMS_CMA_HELPER
 	help
 	  Choose this if you need the KMS CMA helper functions
 
+config DRM_CMA_FBDEV_BUFFER_NUM
+	int "Cma Fbdev Buffer Number"
+	depends on DRM_KMS_CMA_HELPER
+	default 1
+	help
+	  Defines the buffer number of cma fbdev.  Default is one buffer.
+	  For double buffer please set to 2 and 3 for triple buffer.
+
 source "drivers/gpu/drm/i2c/Kconfig"
 
 config DRM_TDFX
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 5075fae..be66042 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -27,6 +27,12 @@
 
 #define DEFAULT_FBDEFIO_DELAY_MS 50
 
+#ifdef CONFIG_DRM_CMA_FBDEV_BUFFER_NUM
+#define FBDEV_BUFFER_NUM CONFIG_DRM_CMA_FBDEV_BUFFER_NUM
+#else
+#define FBDEV_BUFFER_NUM 1
+#endif
+
 struct drm_fb_cma {
 	struct drm_framebuffer		fb;
 	struct drm_gem_cma_object	*obj[4];
@@ -389,7 +395,7 @@ int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
 	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
 
 	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
+	mode_cmd.height = sizes->surface_height * FBDEV_BUFFER_NUM;
 	mode_cmd.pitches[0] = sizes->surface_width * bytes_per_pixel;
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 		sizes->surface_depth);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stefan Christ]

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
+ */
+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;
+
+	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;
+
+			ret = drm_crtc_vblank_get(crtc);
+			if (!ret) {
+				drm_crtc_wait_one_vblank(crtc);
+				drm_crtc_vblank_put(crtc);
+			}
+		}
+		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

[PATCH 3/3] drm/imx: ipuv3-crtc: implement fast path mode_set_base

[Stefan Christ]

Implementing the function "mode_set_base" for i.MX6 IPU enables the
fast-path in function drm_crtc_helper_set_config. The fast-path is used
when flag 'mode_changed' is false and flag 'fb_changed' is true.

The fast-patch is needed for applications using the legcay framebuffer
ioctl FBIOPAN_DISPLAY for buffer flipping. When the fast-patch is not
present, the ioctl FBIOPAN_DISPLAY causes a complete mode set which is
too interruptive.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index fc04041..9858a57 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -234,6 +234,15 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.page_flip = ipu_page_flip,
 };
 
+int ipu_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
+				  struct drm_framebuffer *old_fb)
+{
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+	struct ipu_plane *plane = ipu_crtc->plane[0];
+
+	return ipu_plane_set_base(plane, crtc->primary->fb, x, y);
+}
+
 static int ipu_crtc_mode_set(struct drm_crtc *crtc,
 			       struct drm_display_mode *orig_mode,
 			       struct drm_display_mode *mode,
@@ -378,6 +387,7 @@ static const struct drm_crtc_helper_funcs ipu_helper_funcs = {
 	.dpms = ipu_crtc_dpms,
 	.mode_fixup = ipu_crtc_mode_fixup,
 	.mode_set = ipu_crtc_mode_set,
+	.mode_set_base = ipu_crtc_mode_set_base,
 	.prepare = ipu_crtc_prepare,
 	.commit = ipu_crtc_commit,
 };
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Support fast framebuffer panning for i.MX6

[Daniel Vetter]

On Wed, Jul 13, 2016 at 10:11:45AM +0200, Stefan Christ wrote:
> Hi,
> 
> 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.
> 
> Any comments?

Don't ever do OpenGL on fbdev. Ever. The fbdev emulation we have is to
support boot splashs, kernel console, oopses and legacy applications that
just love fbdev too much and don't support native kms dumb buffers.

Anything that goes beyond kms dumb buffers (like displaying buffers
rendered through opengl) is imo a complete no-go. Yes Android loves to do
that, but for upstream we need a proper drm render driver, buffer sharing
through prime and the userspace hwcomposer needs to use native drm kms
ioctls. Also, userspace (especially the opengl part) needs to be open
source for upstream.

Thanks, Daniel
> 
> Kind regards,
> 	Stefan Christ
> 
> Stefan Christ (2):
>   drm: fb_helper: implement ioctl FBIO_WAITFORVSYNC
>   drm/imx: ipuv3-crtc: implement fast path mode_set_base
> 
> Xinliang Liu (1):
>   drm/cma-helper: Add multi buffer support for cma fbdev
> 
>  drivers/gpu/drm/Kconfig             |  8 +++++++
>  drivers/gpu/drm/drm_fb_cma_helper.c |  9 +++++++-
>  drivers/gpu/drm/drm_fb_helper.c     | 43 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/ipuv3-crtc.c    | 10 +++++++++
>  include/drm/drm_fb_helper.h         |  2 ++
>  5 files changed, 71 insertions(+), 1 deletion(-)
> 
> -- 
> 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

Re: [PATCH 1/3] drm/cma-helper: Add multi buffer support for cma fbdev

[Daniel Vetter]

On Wed, Jul 13, 2016 at 10:11:46AM +0200, Stefan Christ wrote:
> From: Xinliang Liu <xinliang.liu@linaro.org>
> 
> This patch add a config to support to create multi buffer for cma fbdev.
> Such as double buffer and triple buffer.
> 
> Cma fbdev is convient to add a legency fbdev. And still many Android
> devices use fbdev now and at least double buffer is needed for these
> Android devices, so that a buffer flip can be operated. It will need
> some time for Android device vendors to abondon legency fbdev. So multi
> buffer for fbdev is needed.
> 
> Signed-off-by: Xinliang Liu <xinliang.liu@linaro.org>
> [s.christ@phytec.de: Picking patch from
>                      https://lkml.org/lkml/2015/9/14/188]
> Signed-off-by: Stefan Christ <s.christ@phytec.de>

We first need to discuss whether we want to do this as a high level idea,
but meanwhile also some comments about details on the patches.

> ---
>  drivers/gpu/drm/Kconfig             | 8 ++++++++
>  drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index fc35731..56cf34d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -106,6 +106,14 @@ config DRM_KMS_CMA_HELPER
>  	help
>  	  Choose this if you need the KMS CMA helper functions
>  
> +config DRM_CMA_FBDEV_BUFFER_NUM
> +	int "Cma Fbdev Buffer Number"
> +	depends on DRM_KMS_CMA_HELPER
> +	default 1
> +	help
> +	  Defines the buffer number of cma fbdev.  Default is one buffer.
> +	  For double buffer please set to 2 and 3 for triple buffer.
> +
>  source "drivers/gpu/drm/i2c/Kconfig"
>  
>  config DRM_TDFX
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 5075fae..be66042 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -27,6 +27,12 @@
>  
>  #define DEFAULT_FBDEFIO_DELAY_MS 50
>  
> +#ifdef CONFIG_DRM_CMA_FBDEV_BUFFER_NUM
> +#define FBDEV_BUFFER_NUM CONFIG_DRM_CMA_FBDEV_BUFFER_NUM
> +#else
> +#define FBDEV_BUFFER_NUM 1
> +#endif

Imo this should be done in the core helpers, we could do this by
multiplying the height of buffers. And I think we should also expose this
as a module option, with Kconfig simply setting the default.
-Daniel

> +
>  struct drm_fb_cma {
>  	struct drm_framebuffer		fb;
>  	struct drm_gem_cma_object	*obj[4];
> @@ -389,7 +395,7 @@ int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
>  	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
>  
>  	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> +	mode_cmd.height = sizes->surface_height * FBDEV_BUFFER_NUM;
>  	mode_cmd.pitches[0] = sizes->surface_width * bytes_per_pixel;
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>  		sizes->surface_depth);
> -- 
> 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

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

[Daniel Vetter]

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

Re: [PATCH 0/3] Support fast framebuffer panning for i.MX6

[Stefan Christ]

Hi Daniel,

On Wed, Jul 13, 2016 at 12:00:07PM +0200, Daniel Vetter wrote:
> On Wed, Jul 13, 2016 at 10:11:45AM +0200, Stefan Christ wrote:
> > Hi,
> > 
> > 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.
> > 
> > Any comments?
> 
> Don't ever do OpenGL on fbdev. Ever. The fbdev emulation we have is to
> support boot splashs, kernel console, oopses and legacy applications that
> just love fbdev too much and don't support native kms dumb buffers.
> 
> Anything that goes beyond kms dumb buffers (like displaying buffers
> rendered through opengl) is imo a complete no-go. Yes Android loves to do
> that, but for upstream we need a proper drm render driver, buffer sharing
> through prime and the userspace hwcomposer needs to use native drm kms
> ioctls. Also, userspace (especially the opengl part) needs to be open
> source for upstream.

Yeah, these closed libraries are kind of ugly, but implementing these features
maybe interesting for legacy framebuffer applications that do double buffering
via panning.

Thanks for the comments. I totally overlooked the dpms/blank and locking issue.
I will work through them and send v2 patches.

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

> 
> Thanks, Daniel
> > 
> > Kind regards,
> > 	Stefan Christ
> > 
> > Stefan Christ (2):
> >   drm: fb_helper: implement ioctl FBIO_WAITFORVSYNC
> >   drm/imx: ipuv3-crtc: implement fast path mode_set_base
> > 
> > Xinliang Liu (1):
> >   drm/cma-helper: Add multi buffer support for cma fbdev
> > 
> >  drivers/gpu/drm/Kconfig             |  8 +++++++
> >  drivers/gpu/drm/drm_fb_cma_helper.c |  9 +++++++-
> >  drivers/gpu/drm/drm_fb_helper.c     | 43 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/imx/ipuv3-crtc.c    | 10 +++++++++
> >  include/drm/drm_fb_helper.h         |  2 ++
> >  5 files changed, 71 insertions(+), 1 deletion(-)
> > 
> > -- 
> > 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

Re: [PATCH 0/3] Support fast framebuffer panning for i.MX6

[Daniel Vetter]

On Thu, Jul 14, 2016 at 05:11:38PM +0200, Stefan Christ wrote:
> Hi Daniel,
> 
> On Wed, Jul 13, 2016 at 12:00:07PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 13, 2016 at 10:11:45AM +0200, Stefan Christ wrote:
> > > Hi,
> > > 
> > > 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.
> > > 
> > > Any comments?
> > 
> > Don't ever do OpenGL on fbdev. Ever. The fbdev emulation we have is to
> > support boot splashs, kernel console, oopses and legacy applications that
> > just love fbdev too much and don't support native kms dumb buffers.
> > 
> > Anything that goes beyond kms dumb buffers (like displaying buffers
> > rendered through opengl) is imo a complete no-go. Yes Android loves to do
> > that, but for upstream we need a proper drm render driver, buffer sharing
> > through prime and the userspace hwcomposer needs to use native drm kms
> > ioctls. Also, userspace (especially the opengl part) needs to be open
> > source for upstream.
> 
> Yeah, these closed libraries are kind of ugly, but implementing these features
> maybe interesting for legacy framebuffer applications that do double buffering
> via panning.
> 
> Thanks for the comments. I totally overlooked the dpms/blank and locking issue.
> I will work through them and send v2 patches.

In case it wasn't obvious: Don't sell this with "makes blob opengl drivers
work". Find some old userspace that uses raw fbdev, sell this patch series
using that one ;-)
-Daniel

> 
> Mit freundlichen Grüßen / Kind regards,
> 	Stefan Christ
> 
> > 
> > Thanks, Daniel
> > > 
> > > Kind regards,
> > > 	Stefan Christ
> > > 
> > > Stefan Christ (2):
> > >   drm: fb_helper: implement ioctl FBIO_WAITFORVSYNC
> > >   drm/imx: ipuv3-crtc: implement fast path mode_set_base
> > > 
> > > Xinliang Liu (1):
> > >   drm/cma-helper: Add multi buffer support for cma fbdev
> > > 
> > >  drivers/gpu/drm/Kconfig             |  8 +++++++
> > >  drivers/gpu/drm/drm_fb_cma_helper.c |  9 +++++++-
> > >  drivers/gpu/drm/drm_fb_helper.c     | 43 +++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/imx/ipuv3-crtc.c    | 10 +++++++++
> > >  include/drm/drm_fb_helper.h         |  2 ++
> > >  5 files changed, 71 insertions(+), 1 deletion(-)
> > > 
> > > -- 
> > > 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

-- 
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

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

[Daniel Vetter]

On Wed, Jul 13, 2016 at 12:16:05PM +0200, Daniel Vetter wrote:
> 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.

To roll new vfuncs out consistently I think it'd be great to create a
DRM_FB_HELPER_DEFAULT_OPS #define which sets all the fb_ops. Drivers can
then overwrite just what they need, e.g.

static struct fb_ops drm_fbdev_cma_ops = {
	.owner		= THIS_MODULE,
	DRM_FB_HELPER_DEFAULT_OPS,
	.fb_mmap 	= drm_fb_cma_mmap,
};

Maybe even include the .owner = THIS_MODULE line in the macro, but that
might be too much magic and mislead people into reusing it when it's not
the same module.

Of course this would be an additional (subsystem-wide) prep patch before
this one here.
-Daniel
-- 
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

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

[Stefan Christ]

Hi Daniel,

I finally found some time to look into this issue again.

On Fri, Jul 15, 2016 at 09:19:22AM +0200, Daniel Vetter wrote:
> <snippet>
> 
> To roll new vfuncs out consistently I think it'd be great to create a
> DRM_FB_HELPER_DEFAULT_OPS #define which sets all the fb_ops. Drivers can
> then overwrite just what they need, e.g.
> 
> static struct fb_ops drm_fbdev_cma_ops = {
> 	.owner		= THIS_MODULE,
> 	DRM_FB_HELPER_DEFAULT_OPS,
> 	.fb_mmap 	= drm_fb_cma_mmap,
> };
> 
> Maybe even include the .owner = THIS_MODULE line in the macro, but that
> might be too much magic and mislead people into reusing it when it's not
> the same module.
> 
> Of course this would be an additional (subsystem-wide) prep patch before
> this one here.
> -Daniel

Ok. This pattern seems like single inheritance in object-oriented programming
languages ;-) The define looks very reasonable since these drm helper functions
are used in lot of drivers.

I would also not include '.owner = THIS_MODULE' in the define, because it would
contain even more magic.

What is the preferred way for subsystem-wide patches? One big patch, one patch
per driver or group of drivers per patch?

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

> -- 
> 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

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

[Daniel Vetter]

On Wed, Jul 27, 2016 at 11:59:10AM +0200, Stefan Christ wrote:
> Hi Daniel,
> 
> I finally found some time to look into this issue again.
> 
> On Fri, Jul 15, 2016 at 09:19:22AM +0200, Daniel Vetter wrote:
> > <snippet>
> > 
> > To roll new vfuncs out consistently I think it'd be great to create a
> > DRM_FB_HELPER_DEFAULT_OPS #define which sets all the fb_ops. Drivers can
> > then overwrite just what they need, e.g.
> > 
> > static struct fb_ops drm_fbdev_cma_ops = {
> > 	.owner		= THIS_MODULE,
> > 	DRM_FB_HELPER_DEFAULT_OPS,
> > 	.fb_mmap 	= drm_fb_cma_mmap,
> > };
> > 
> > Maybe even include the .owner = THIS_MODULE line in the macro, but that
> > might be too much magic and mislead people into reusing it when it's not
> > the same module.
> > 
> > Of course this would be an additional (subsystem-wide) prep patch before
> > this one here.
> > -Daniel
> 
> Ok. This pattern seems like single inheritance in object-oriented programming
> languages ;-) The define looks very reasonable since these drm helper functions
> are used in lot of drivers.
> 
> I would also not include '.owner = THIS_MODULE' in the define, because it would
> contain even more magic.

Agreed, that's why I left it out in the example.

> What is the preferred way for subsystem-wide patches? One big patch, one patch
> per driver or group of drivers per patch?

Since this patch is not a flag day I'd say 1 patch to create the #define,
1 patch per driver (with maintainers/mailing lists per
scripts/get_maintainers.pl on cc) for each driver. It's a bit busy work,
but easiest to do that way. For flag-day changes you kinda have to do 1
huge patch, but that easily runs into cc limits of various mailing lists.
-Daniel
-- 
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