[PATCH v3 0/2] drm: Support framebuffer panning

[PATCH v3 0/2] 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 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.

Stefan Christ (1):
  drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

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

 drivers/gpu/drm/Kconfig         |  9 ++++-
 drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     | 12 +++++-
 3 files changed, 93 insertions(+), 1 deletion(-)

base-commit: 0879b944c95d65f7523d178d3addaebe84e510ec
-- 
git-series 0.8.11

[PATCH v3 0/2] 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 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.

Stefan Christ (1):
  drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC

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

 drivers/gpu/drm/Kconfig         |  9 ++++-
 drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     | 12 +++++-
 3 files changed, 93 insertions(+), 1 deletion(-)

base-commit: 0879b944c95d65f7523d178d3addaebe84e510ec
-- 
git-series 0.8.11
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maxime Ripard]

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>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/Kconfig         |  9 +++++++++
 drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index ebfe8404c25f..700c8b8e57a9 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
 
 	  If in doubt, say "Y".
 
+config DRM_FBDEV_OVERALLOC
+	int "Overallocation of the fbdev buffer"
+	depends on DRM_FBDEV_EMULATION
+	default 100
+	help
+	  Defines the fbdev buffer overallocation in percent. Default
+	  is 100. Typical values for double buffering will be 200,
+	  triple buffering 300.
+
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
 	depends on DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e934b541feea..c6de87abaca8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
 MODULE_PARM_DESC(fbdev_emulation,
 		 "Enable legacy fbdev emulation [default=true]");
 
+static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
+module_param(drm_fbdev_overalloc, int, 0444);
+MODULE_PARM_DESC(drm_fbdev_overalloc,
+		 "Overallocation of the fbdev buffer (%) [default="
+		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
+
 static LIST_HEAD(kernel_fb_helper_list);
 static DEFINE_MUTEX(kernel_fb_helper_lock);
 
@@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		sizes.fb_height = sizes.surface_height = 768;
 	}
 
+	/* Handle our overallocation */
+	sizes.surface_height *= drm_fbdev_overalloc;
+	sizes.surface_height /= 100;
+
 	/* push down into drivers */
 	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
 	if (ret < 0)
-- 
git-series 0.8.11

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

[Maxime Ripard]

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>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/Kconfig         |  9 +++++++++
 drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index ebfe8404c25f..700c8b8e57a9 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
 
 	  If in doubt, say "Y".
 
+config DRM_FBDEV_OVERALLOC
+	int "Overallocation of the fbdev buffer"
+	depends on DRM_FBDEV_EMULATION
+	default 100
+	help
+	  Defines the fbdev buffer overallocation in percent. Default
+	  is 100. Typical values for double buffering will be 200,
+	  triple buffering 300.
+
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
 	depends on DRM_KMS_HELPER
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e934b541feea..c6de87abaca8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
 MODULE_PARM_DESC(fbdev_emulation,
 		 "Enable legacy fbdev emulation [default=true]");
 
+static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
+module_param(drm_fbdev_overalloc, int, 0444);
+MODULE_PARM_DESC(drm_fbdev_overalloc,
+		 "Overallocation of the fbdev buffer (%) [default="
+		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
+
 static LIST_HEAD(kernel_fb_helper_list);
 static DEFINE_MUTEX(kernel_fb_helper_lock);
 
@@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		sizes.fb_height = sizes.surface_height = 768;
 	}
 
+	/* Handle our overallocation */
+	sizes.surface_height *= drm_fbdev_overalloc;
+	sizes.surface_height /= 100;
+
 	/* push down into drivers */
 	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
 	if (ret < 0)
-- 
git-series 0.8.11
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stefan Lengfeld]

Hi Maxime,

sorry, I have missed the discussion about the double buffering/virtual
surface size patch series two weeks ago. My comments about the patch are
inline:

On Wed, Feb 15, 2017 at 05:19:08PM +0100, Maxime Ripard wrote:
> From: Xinliang Liu <xinliang.liu@linaro.org>

Mabye you should take the authorship here. Taking the credit and the
blame, because the patch was heavily modified by you and me. But I don't
really know what the offical police about that is.

> 
> 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>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu>

My surname has changed from "Christ" to "Lengfeld" recently. So my
review tag contains the new name.

> ---
>  drivers/gpu/drm/Kconfig         |  9 +++++++++
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index ebfe8404c25f..700c8b8e57a9 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
>  
>  	  If in doubt, say "Y".
>  
> +config DRM_FBDEV_OVERALLOC
> +	int "Overallocation of the fbdev buffer"
> +	depends on DRM_FBDEV_EMULATION
> +	default 100
> +	help
> +	  Defines the fbdev buffer overallocation in percent. Default
> +	  is 100. Typical values for double buffering will be 200,
> +	  triple buffering 300.
> +
>  config DRM_LOAD_EDID_FIRMWARE
>  	bool "Allow to specify an EDID data set instead of probing for it"
>  	depends on DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e934b541feea..c6de87abaca8 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>  MODULE_PARM_DESC(fbdev_emulation,
>  		 "Enable legacy fbdev emulation [default=true]");
>  
> +static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
> +module_param(drm_fbdev_overalloc, int, 0444);

Maybe the variable should be of type "uint" instead of "int". This would
rule out the negative numbers error case.

> +MODULE_PARM_DESC(drm_fbdev_overalloc,
> +		 "Overallocation of the fbdev buffer (%) [default="
> +		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> +
>  static LIST_HEAD(kernel_fb_helper_list);
>  static DEFINE_MUTEX(kernel_fb_helper_lock);
>  
> @@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  		sizes.fb_height = sizes.surface_height = 768;
>  	}
>  
> +	/* Handle our overallocation */
> +	sizes.surface_height *= drm_fbdev_overalloc;
> +	sizes.surface_height /= 100;
> +

The code can trigger an arithmetic overflow, but I think we can ignore
this error case here.

But there should be a check for drm_fbdev_overalloc not be smaller than
100. If it is smaller, the variable drm_fbdev_overalloc should have the
default value "100". Otherwise the virtual surface height can be smaller
than the physical height. This could trigger a lot of errors in existing
code paths.

Kind regards,
	Stefan Lengfeld

>  	/* push down into drivers */
>  	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
>  	if (ret < 0)
> -- 
> git-series 0.8.11
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4404 bytes --]

Hi Stefan,

On Mon, Feb 20, 2017 at 06:07:10PM +0100, Stefan Lengfeld wrote:
> Hi Maxime,
> 
> sorry, I have missed the discussion about the double buffering/virtual
> surface size patch series two weeks ago. My comments about the patch are
> inline:
> 
> On Wed, Feb 15, 2017 at 05:19:08PM +0100, Maxime Ripard wrote:
> > From: Xinliang Liu <xinliang.liu@linaro.org>
> 
> Mabye you should take the authorship here. Taking the credit and the
> blame, because the patch was heavily modified by you and me. But I don't
> really know what the offical police about that is.

I don't usually know when to do that as well. But yeah, you're
probably right.

> > 
> > 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>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu>
> 
> My surname has changed from "Christ" to "Lengfeld" recently. So my
> review tag contains the new name.

Ack

> 
> > ---
> >  drivers/gpu/drm/Kconfig         |  9 +++++++++
> >  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
> >  2 files changed, 19 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index ebfe8404c25f..700c8b8e57a9 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
> >  
> >  	  If in doubt, say "Y".
> >  
> > +config DRM_FBDEV_OVERALLOC
> > +	int "Overallocation of the fbdev buffer"
> > +	depends on DRM_FBDEV_EMULATION
> > +	default 100
> > +	help
> > +	  Defines the fbdev buffer overallocation in percent. Default
> > +	  is 100. Typical values for double buffering will be 200,
> > +	  triple buffering 300.
> > +
> >  config DRM_LOAD_EDID_FIRMWARE
> >  	bool "Allow to specify an EDID data set instead of probing for it"
> >  	depends on DRM_KMS_HELPER
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index e934b541feea..c6de87abaca8 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >  MODULE_PARM_DESC(fbdev_emulation,
> >  		 "Enable legacy fbdev emulation [default=true]");
> >  
> > +static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
> > +module_param(drm_fbdev_overalloc, int, 0444);
> 
> Maybe the variable should be of type "uint" instead of "int". This would
> rule out the negative numbers error case.

Yep, I'll change it.

> > +MODULE_PARM_DESC(drm_fbdev_overalloc,
> > +		 "Overallocation of the fbdev buffer (%) [default="
> > +		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> > +
> >  static LIST_HEAD(kernel_fb_helper_list);
> >  static DEFINE_MUTEX(kernel_fb_helper_lock);
> >  
> > @@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> >  		sizes.fb_height = sizes.surface_height = 768;
> >  	}
> >  
> > +	/* Handle our overallocation */
> > +	sizes.surface_height *= drm_fbdev_overalloc;
> > +	sizes.surface_height /= 100;
> > +
> 
> The code can trigger an arithmetic overflow, but I think we can ignore
> this error case here.
> 
> But there should be a check for drm_fbdev_overalloc not be smaller than
> 100. If it is smaller, the variable drm_fbdev_overalloc should have the
> default value "100". Otherwise the virtual surface height can be smaller
> than the physical height. This could trigger a lot of errors in existing
> code paths.

That's a really good point, I'll change that.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4404 bytes --]

Hi Stefan,

On Mon, Feb 20, 2017 at 06:07:10PM +0100, Stefan Lengfeld wrote:
> Hi Maxime,
> 
> sorry, I have missed the discussion about the double buffering/virtual
> surface size patch series two weeks ago. My comments about the patch are
> inline:
> 
> On Wed, Feb 15, 2017 at 05:19:08PM +0100, Maxime Ripard wrote:
> > From: Xinliang Liu <xinliang.liu@linaro.org>
> 
> Mabye you should take the authorship here. Taking the credit and the
> blame, because the patch was heavily modified by you and me. But I don't
> really know what the offical police about that is.

I don't usually know when to do that as well. But yeah, you're
probably right.

> > 
> > 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>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu>
> 
> My surname has changed from "Christ" to "Lengfeld" recently. So my
> review tag contains the new name.

Ack

> 
> > ---
> >  drivers/gpu/drm/Kconfig         |  9 +++++++++
> >  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
> >  2 files changed, 19 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index ebfe8404c25f..700c8b8e57a9 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
> >  
> >  	  If in doubt, say "Y".
> >  
> > +config DRM_FBDEV_OVERALLOC
> > +	int "Overallocation of the fbdev buffer"
> > +	depends on DRM_FBDEV_EMULATION
> > +	default 100
> > +	help
> > +	  Defines the fbdev buffer overallocation in percent. Default
> > +	  is 100. Typical values for double buffering will be 200,
> > +	  triple buffering 300.
> > +
> >  config DRM_LOAD_EDID_FIRMWARE
> >  	bool "Allow to specify an EDID data set instead of probing for it"
> >  	depends on DRM_KMS_HELPER
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index e934b541feea..c6de87abaca8 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> >  MODULE_PARM_DESC(fbdev_emulation,
> >  		 "Enable legacy fbdev emulation [default=true]");
> >  
> > +static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
> > +module_param(drm_fbdev_overalloc, int, 0444);
> 
> Maybe the variable should be of type "uint" instead of "int". This would
> rule out the negative numbers error case.

Yep, I'll change it.

> > +MODULE_PARM_DESC(drm_fbdev_overalloc,
> > +		 "Overallocation of the fbdev buffer (%) [default="
> > +		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> > +
> >  static LIST_HEAD(kernel_fb_helper_list);
> >  static DEFINE_MUTEX(kernel_fb_helper_lock);
> >  
> > @@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> >  		sizes.fb_height = sizes.surface_height = 768;
> >  	}
> >  
> > +	/* Handle our overallocation */
> > +	sizes.surface_height *= drm_fbdev_overalloc;
> > +	sizes.surface_height /= 100;
> > +
> 
> The code can trigger an arithmetic overflow, but I think we can ignore
> this error case here.
> 
> But there should be a check for drm_fbdev_overalloc not be smaller than
> 100. If it is smaller, the variable drm_fbdev_overalloc should have the
> default value "100". Otherwise the virtual surface height can be smaller
> than the physical height. This could trigger a lot of errors in existing
> code paths.

That's a really good point, I'll change that.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Daniel Vetter]

On Wed, Feb 22, 2017 at 04:49:01PM -0800, Maxime Ripard wrote:
> Hi Stefan,
> 
> On Mon, Feb 20, 2017 at 06:07:10PM +0100, Stefan Lengfeld wrote:
> > Hi Maxime,
> > 
> > sorry, I have missed the discussion about the double buffering/virtual
> > surface size patch series two weeks ago. My comments about the patch are
> > inline:
> > 
> > On Wed, Feb 15, 2017 at 05:19:08PM +0100, Maxime Ripard wrote:
> > > From: Xinliang Liu <xinliang.liu@linaro.org>
> > 
> > Mabye you should take the authorship here. Taking the credit and the
> > blame, because the patch was heavily modified by you and me. But I don't
> > really know what the offical police about that is.
> 
> I don't usually know when to do that as well. But yeah, you're
> probably right.

Sob line is the important bit, retaining original authorship is just
goodwill, and if in doubt showing more curtesy rarely hurts :-)

Patch applied to drm-misc, with Dave's ack after a quick irc discussion.
-Daniel

> 
> > > 
> > > 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>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu>
> > 
> > My surname has changed from "Christ" to "Lengfeld" recently. So my
> > review tag contains the new name.
> 
> Ack
> 
> > 
> > > ---
> > >  drivers/gpu/drm/Kconfig         |  9 +++++++++
> > >  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
> > >  2 files changed, 19 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index ebfe8404c25f..700c8b8e57a9 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION
> > >  
> > >  	  If in doubt, say "Y".
> > >  
> > > +config DRM_FBDEV_OVERALLOC
> > > +	int "Overallocation of the fbdev buffer"
> > > +	depends on DRM_FBDEV_EMULATION
> > > +	default 100
> > > +	help
> > > +	  Defines the fbdev buffer overallocation in percent. Default
> > > +	  is 100. Typical values for double buffering will be 200,
> > > +	  triple buffering 300.
> > > +
> > >  config DRM_LOAD_EDID_FIRMWARE
> > >  	bool "Allow to specify an EDID data set instead of probing for it"
> > >  	depends on DRM_KMS_HELPER
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index e934b541feea..c6de87abaca8 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> > >  MODULE_PARM_DESC(fbdev_emulation,
> > >  		 "Enable legacy fbdev emulation [default=true]");
> > >  
> > > +static int drm_fbdev_overalloc = CONFIG_DRM_FBDEV_OVERALLOC;
> > > +module_param(drm_fbdev_overalloc, int, 0444);
> > 
> > Maybe the variable should be of type "uint" instead of "int". This would
> > rule out the negative numbers error case.
> 
> Yep, I'll change it.
> 
> > > +MODULE_PARM_DESC(drm_fbdev_overalloc,
> > > +		 "Overallocation of the fbdev buffer (%) [default="
> > > +		 __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]");
> > > +
> > >  static LIST_HEAD(kernel_fb_helper_list);
> > >  static DEFINE_MUTEX(kernel_fb_helper_lock);
> > >  
> > > @@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> > >  		sizes.fb_height = sizes.surface_height = 768;
> > >  	}
> > >  
> > > +	/* Handle our overallocation */
> > > +	sizes.surface_height *= drm_fbdev_overalloc;
> > > +	sizes.surface_height /= 100;
> > > +
> > 
> > The code can trigger an arithmetic overflow, but I think we can ignore
> > this error case here.
> > 
> > But there should be a check for drm_fbdev_overalloc not be smaller than
> > 100. If it is smaller, the variable drm_fbdev_overalloc should have the
> > default value "100". Otherwise the virtual surface height can be smaller
> > than the physical height. This could trigger a lot of errors in existing
> > code paths.
> 
> That's a really good point, I'll change that.
> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



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

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

[Maxime Ripard]

From: Stefan Christ <s.christ@phytec.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@phytec.de>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 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;
+		}
+
+		mode_set = &fb_helper->crtc_info[karg].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

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

[Maxime Ripard]

From: Stefan Christ <s.christ@phytec.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@phytec.de>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 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;
+		}
+
+		mode_set = &fb_helper->crtc_info[karg].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

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@phytec.de>
> 

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@phytec.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  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

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@phytec.de>
> 

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@phytec.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> Hi Maxime,
> 
> On Wed, Feb 15, 2017 at 05:19:09PM +0100, Maxime Ripard wrote:
> > From: Stefan Christ <s.christ@phytec.de>
> > 
> 
> 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@phytec.de>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  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'.

Have a look at matroxfb.

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

Again the matrox driver is different. And looks like unichrome might have
done something even more exotic with the argument.

But I do agree that using this with the kms fbdev implementation could
be quite problematic as you can't actually know which crtcs the
fb_helper has picked.

> 
> 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
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> Hi Maxime,
> 
> On Wed, Feb 15, 2017 at 05:19:09PM +0100, Maxime Ripard wrote:
> > From: Stefan Christ <s.christ@phytec.de>
> > 
> 
> 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@phytec.de>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  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'.

Have a look at matroxfb.

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

Again the matrox driver is different. And looks like unichrome might have
done something even more exotic with the argument.

But I do agree that using this with the kms fbdev implementation could
be quite problematic as you can't actually know which crtcs the
fb_helper has picked.

> 
> 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
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4190 bytes --]

Hi Ville, Stefan,

On Tue, Feb 21, 2017 at 12:55:01PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> > Hi Maxime,
> > 
> > On Wed, Feb 15, 2017 at 05:19:09PM +0100, Maxime Ripard wrote:
> > > From: Stefan Christ <s.christ@phytec.de>
> > > 
> > 
> > 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@phytec.de>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > >  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'.
> 
> Have a look at matroxfb.
> 
> > 
> > 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 ))
> 
> Again the matrox driver is different. And looks like unichrome might have
> done something even more exotic with the argument.
> 
> But I do agree that using this with the kms fbdev implementation could
> be quite problematic as you can't actually know which crtcs the
> fb_helper has picked.

I'm not sure what a good solution might be then. Is just waiting for
always the first crtc acceptable? All the code I could find is passing
0 as an argument anyway, so that's effectively the same behaviour
anyway here. And getting tearing might be a good argument in favour of
moving to KMS :)

Thanks
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4190 bytes --]

Hi Ville, Stefan,

On Tue, Feb 21, 2017 at 12:55:01PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> > Hi Maxime,
> > 
> > On Wed, Feb 15, 2017 at 05:19:09PM +0100, Maxime Ripard wrote:
> > > From: Stefan Christ <s.christ@phytec.de>
> > > 
> > 
> > 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@phytec.de>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > >  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'.
> 
> Have a look at matroxfb.
> 
> > 
> > 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 ))
> 
> Again the matrox driver is different. And looks like unichrome might have
> done something even more exotic with the argument.
> 
> But I do agree that using this with the kms fbdev implementation could
> be quite problematic as you can't actually know which crtcs the
> fb_helper has picked.

I'm not sure what a good solution might be then. Is just waiting for
always the first crtc acceptable? All the code I could find is passing
0 as an argument anyway, so that's effectively the same behaviour
anyway here. And getting tearing might be a good argument in favour of
moving to KMS :)

Thanks
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Stefan Lengfeld]

Hi Maxime, Hi Ville,

On Wed, Feb 22, 2017 at 04:52:33PM -0800, Maxime Ripard wrote:
> Hi Ville, Stefan,
> 
> On Tue, Feb 21, 2017 at 12:55:01PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> > > 
> > > 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 ))
> > 
> > Again the matrox driver is different. And looks like unichrome might have
> > done something even more exotic with the argument.
> > 
> > But I do agree that using this with the kms fbdev implementation could
> > be quite problematic as you can't actually know which crtcs the
> > fb_helper has picked.
> 
> I'm not sure what a good solution might be then. Is just waiting for
> always the first crtc acceptable? All the code I could find is passing
> 0 as an argument anyway, so that's effectively the same behaviour
> anyway here. And getting tearing might be a good argument in favour of
> moving to KMS :)
> 

I also vote for the first solution: just waiting for the first enabled
crtc. It's deterministic and userspace can avoid tearing at least for
one crtc. If the system has more active scanouts at the same time with
different timings, userspace has to use the newer kms/drm API.

It is the same argument as already posted by Michel Dänzer:

     > 2/ Wait for a single vsync event on all active crtcs as Ville Syrjälä
     >    described here [2] in the optimal implementation.
     
     FWIW, this seems like a bad idea, since with multiple active CRTCs it
     would make it essentially random at which intervals the ioctl returns,
     and which CRTCs are in vertical blank when it does. So it would be
     useless for both timing and tearing prevention purposes, i.e. more or
     less completely useless.

So I think the implementation (waiting only for the first crtc) of ioctl
FBIO_WAITFORVSYNC is a good enough implementation and works the most
already existing userspace code out there.

Kind regards,
    Stefan Lengfeld

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

[Daniel Vetter]

On Thu, Feb 23, 2017 at 10:02:26AM +0100, Stefan Lengfeld wrote:
> Hi Maxime, Hi Ville,
> 
> On Wed, Feb 22, 2017 at 04:52:33PM -0800, Maxime Ripard wrote:
> > Hi Ville, Stefan,
> > 
> > On Tue, Feb 21, 2017 at 12:55:01PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 21, 2017 at 11:00:59AM +0100, Stefan Lengfeld wrote:
> > > > 
> > > > 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 ))
> > > 
> > > Again the matrox driver is different. And looks like unichrome might have
> > > done something even more exotic with the argument.
> > > 
> > > But I do agree that using this with the kms fbdev implementation could
> > > be quite problematic as you can't actually know which crtcs the
> > > fb_helper has picked.
> > 
> > I'm not sure what a good solution might be then. Is just waiting for
> > always the first crtc acceptable? All the code I could find is passing
> > 0 as an argument anyway, so that's effectively the same behaviour
> > anyway here. And getting tearing might be a good argument in favour of
> > moving to KMS :)
> > 
> 
> I also vote for the first solution: just waiting for the first enabled
> crtc. It's deterministic and userspace can avoid tearing at least for
> one crtc. If the system has more active scanouts at the same time with
> different timings, userspace has to use the newer kms/drm API.
> 
> It is the same argument as already posted by Michel Dänzer:
> 
>      > 2/ Wait for a single vsync event on all active crtcs as Ville Syrjälä
>      >    described here [2] in the optimal implementation.
>      
>      FWIW, this seems like a bad idea, since with multiple active CRTCs it
>      would make it essentially random at which intervals the ioctl returns,
>      and which CRTCs are in vertical blank when it does. So it would be
>      useless for both timing and tearing prevention purposes, i.e. more or
>      less completely useless.
> 
> So I think the implementation (waiting only for the first crtc) of ioctl
> FBIO_WAITFORVSYNC is a good enough implementation and works the most
> already existing userspace code out there.

Also note that integrated panels /should/ be the first connectors in the
connector list, which means the first active crtc is most likely the
integrated panel (if there is one).

That's about as good a default choice as we can make I think, everything
else really just needs to get over to doing KMS natively.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Michel Dänzer]

On 21/02/17 07:00 PM, Stefan Lengfeld wrote:
> 
> 2/ Wait for a single vsync event on all active crtcs as Ville Syrjälä 
>    described here [2] in the optimal implementation.

FWIW, this seems like a bad idea, since with multiple active CRTCs it
would make it essentially random at which intervals the ioctl returns,
and which CRTCs are in vertical blank when it does. So it would be
useless for both timing and tearing prevention purposes, i.e. more or
less completely useless.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

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

[Michel Dänzer]

On 21/02/17 07:00 PM, Stefan Lengfeld wrote:
> 
> 2/ Wait for a single vsync event on all active crtcs as Ville Syrjälä 
>    described here [2] in the optimal implementation.

FWIW, this seems like a bad idea, since with multiple active CRTCs it
would make it essentially random at which intervals the ioctl returns,
and which CRTCs are in vertical blank when it does. So it would be
useless for both timing and tearing prevention purposes, i.e. more or
less completely useless.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/2] drm: Support framebuffer panning

[Neil Armstrong]

On 02/15/2017 05:19 PM, Maxime Ripard wrote:
> 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 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.
> 
> Stefan Christ (1):
>   drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC
> 
> Xinliang Liu (1):
>   drm/fb-helper: Add multi buffer support for cma fbdev
> 
>  drivers/gpu/drm/Kconfig         |  9 ++++-
>  drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     | 12 +++++-
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> base-commit: 0879b944c95d65f7523d178d3addaebe84e510ec
> 
Hi Maxime,

Just tested these patches on Amlogic GXBB to provide 3D Mali acceleration on framebuffer.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks,
Neil

[PATCH v3 0/2] drm: Support framebuffer panning

[Neil Armstrong]

On 02/15/2017 05:19 PM, Maxime Ripard wrote:
> 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 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.
> 
> Stefan Christ (1):
>   drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC
> 
> Xinliang Liu (1):
>   drm/fb-helper: Add multi buffer support for cma fbdev
> 
>  drivers/gpu/drm/Kconfig         |  9 ++++-
>  drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     | 12 +++++-
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> base-commit: 0879b944c95d65f7523d178d3addaebe84e510ec
> 
Hi Maxime,

Just tested these patches on Amlogic GXBB to provide 3D Mali acceleration on framebuffer.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks,
Neil

Re: [PATCH v3 0/2] drm: Support framebuffer panning

[Neil Armstrong]

On 02/15/2017 05:19 PM, Maxime Ripard wrote:
> 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 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.
> 
> Stefan Christ (1):
>   drm/fb-helper: implement ioctl FBIO_WAITFORVSYNC
> 
> Xinliang Liu (1):
>   drm/fb-helper: Add multi buffer support for cma fbdev
> 
>  drivers/gpu/drm/Kconfig         |  9 ++++-
>  drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     | 12 +++++-
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> base-commit: 0879b944c95d65f7523d178d3addaebe84e510ec
> 
Hi Maxime,

Just tested these patches on Amlogic GXBB to provide 3D Mali acceleration on framebuffer.

Tested-by: Neil Armstrong <narmstrong@baylibre.com>

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