Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572263 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] drm: Support framebuffer panning Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-02-02 11:40 +0100
    [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-02-02 11:40 +0100
      Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Daniel Vetter <daniel@ffwll.ch> - 2017-02-09 18:10 +0100
        Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Daniel Vetter <daniel@ffwll.ch> - 2017-02-09 20:10 +0100
        Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Daniel Stone <daniel@fooishbar.org> - 2017-02-10 00:00 +0100
      Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-02-10 15:30 +0100
        Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-02-13 11:40 +0100
          Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-02-13 15:50 +0100
            Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-02-15 15:10 +0100

#1572263 — [PATCH v2 0/2] drm: Support framebuffer panning

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 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/cma-helper: Add multi buffer support for cma fbdev

 drivers/gpu/drm/Kconfig             |  8 ++++-
 drivers/gpu/drm/drm_fb_cma_helper.c |  8 +++-
 drivers/gpu/drm/drm_fb_helper.c     | 55 ++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h         |  5 ++-
 4 files changed, 74 insertions(+), 2 deletions(-)

base-commit: 8b394ae674998d4ade6cb48aec3fca5445908dfe
-- 
git-series 0.8.11

[toc] | [next] | [standalone]

#1572265 — [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

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 | 55 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     |  5 ++-
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e934b541feea..39a3532e311c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
 
 /**
+ * drm_fb_helper_ioctl - legacy ioctl implementation
+ * @info: fbdev registered by the helper
+ * @cmd: ioctl command
+ * @arg: ioctl argument
+ *
+ * A helper to implement the standard fbdev ioctl. Only
+ * FBIO_WAITFORVSYNC is implemented for now.
+ */
+int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	struct drm_device *dev = fb_helper->dev;
+	unsigned int i;
+	int ret = 0;
+
+	drm_modeset_lock_all(dev);
+	if (!drm_fb_helper_is_bound(fb_helper)) {
+		drm_modeset_unlock_all(dev);
+		return -EBUSY;
+	}
+
+	switch (cmd) {
+	case FBIO_WAITFORVSYNC:
+		for (i = 0; i < fb_helper->crtc_count; i++) {
+			struct drm_mode_set *mode_set;
+			struct drm_crtc *crtc;
+
+			mode_set = &fb_helper->crtc_info[i].mode_set;
+			crtc = mode_set->crtc;
+
+			/*
+			 * Only call drm_crtc_wait_one_vblank for crtcs that
+			 * are currently enabled.
+			 */
+			if (!crtc->enabled)
+				continue;
+
+			ret = drm_crtc_vblank_get(crtc);
+			if (!ret) {
+				drm_crtc_wait_one_vblank(crtc);
+				drm_crtc_vblank_put(crtc);
+			}
+		}
+		goto unlock;
+	default:
+		ret = -ENOTTY;
+	}
+
+unlock:
+	drm_modeset_unlock_all(dev);
+	return ret;
+}
+EXPORT_SYMBOL(drm_fb_helper_ioctl);
+
+/**
  * drm_fb_helper_check_var - implementation for ->fb_check_var
  * @var: screeninfo to check
  * @info: fbdev registered by the helper
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 975deedd593e..460be9d9fa98 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -230,7 +230,8 @@ struct drm_fb_helper {
 	.fb_blank	= drm_fb_helper_blank, \
 	.fb_pan_display	= drm_fb_helper_pan_display, \
 	.fb_debug_enter = drm_fb_helper_debug_enter, \
-	.fb_debug_leave = drm_fb_helper_debug_leave
+	.fb_debug_leave = drm_fb_helper_debug_leave, \
+	.fb_ioctl	= drm_fb_helper_ioctl
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
@@ -286,6 +287,8 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
 
 int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
 
+int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg);
+
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
 int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel);
 int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper);
-- 
git-series 0.8.11

[toc] | [prev] | [next] | [standalone]

#1577787 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> From: Stefan Christ <s.christ@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 | 55 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     |  5 ++-
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e934b541feea..39a3532e311c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
>  
>  /**
> + * drm_fb_helper_ioctl - legacy ioctl implementation
> + * @info: fbdev registered by the helper
> + * @cmd: ioctl command
> + * @arg: ioctl argument
> + *
> + * A helper to implement the standard fbdev ioctl. Only
> + * FBIO_WAITFORVSYNC is implemented for now.
> + */
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_device *dev = fb_helper->dev;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	drm_modeset_lock_all(dev);
> +	if (!drm_fb_helper_is_bound(fb_helper)) {
> +		drm_modeset_unlock_all(dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (cmd) {
> +	case FBIO_WAITFORVSYNC:
> +		for (i = 0; i < fb_helper->crtc_count; i++) {
> +			struct drm_mode_set *mode_set;
> +			struct drm_crtc *crtc;
> +
> +			mode_set = &fb_helper->crtc_info[i].mode_set;
> +			crtc = mode_set->crtc;
> +
> +			/*
> +			 * Only call drm_crtc_wait_one_vblank for crtcs that
> +			 * are currently enabled.
> +			 */
> +			if (!crtc->enabled)
> +				continue;

This needs locking, and the interface spec for vblank_get says you'll get
an -EINVAL if the pipe is off. I'd say drop this, and instead eat the
-EINVAL from vblank_get and explain in a comment why you do that.

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

Otherwise lgtm.
-Daniel

> +
> +			ret = drm_crtc_vblank_get(crtc);
> +			if (!ret) {
> +				drm_crtc_wait_one_vblank(crtc);
> +				drm_crtc_vblank_put(crtc);
> +			}
> +		}
> +		goto unlock;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +unlock:
> +	drm_modeset_unlock_all(dev);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_ioctl);
> +
> +/**
>   * drm_fb_helper_check_var - implementation for ->fb_check_var
>   * @var: screeninfo to check
>   * @info: fbdev registered by the helper
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 975deedd593e..460be9d9fa98 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -230,7 +230,8 @@ struct drm_fb_helper {
>  	.fb_blank	= drm_fb_helper_blank, \
>  	.fb_pan_display	= drm_fb_helper_pan_display, \
>  	.fb_debug_enter = drm_fb_helper_debug_enter, \
> -	.fb_debug_leave = drm_fb_helper_debug_leave
> +	.fb_debug_leave = drm_fb_helper_debug_leave, \
> +	.fb_ioctl	= drm_fb_helper_ioctl
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
> @@ -286,6 +287,8 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
>  
>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
>  
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg);
> +
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel);
>  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper);
> -- 
> git-series 0.8.11
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[toc] | [prev] | [next] | [standalone]

#1577886 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

On Thu, Feb 09, 2017 at 05:38:26PM +0000, Daniel Stone wrote:
> Hi,
> 
> On 9 February 2017 at 17:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> >> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> >> +{
> >> +     struct drm_fb_helper *fb_helper = info->par;
> >> +     struct drm_device *dev = fb_helper->dev;
> >> +     unsigned int i;
> >> +     int ret = 0;
> >> +
> >> +     drm_modeset_lock_all(dev);
> >> +     if (!drm_fb_helper_is_bound(fb_helper)) {
> >> +             drm_modeset_unlock_all(dev);
> >> +             return -EBUSY;
> >> +     }
> >> +
> >> +     switch (cmd) {
> >> +     case FBIO_WAITFORVSYNC:
> >> +             for (i = 0; i < fb_helper->crtc_count; i++) {
> >> +                     struct drm_mode_set *mode_set;
> >> +                     struct drm_crtc *crtc;
> >> +
> >> +                     mode_set = &fb_helper->crtc_info[i].mode_set;
> >> +                     crtc = mode_set->crtc;
> >> +
> >> +                     /*
> >> +                      * Only call drm_crtc_wait_one_vblank for crtcs that
> >> +                      * are currently enabled.
> >> +                      */
> >> +                     if (!crtc->enabled)
> >> +                             continue;
> >
> > This needs locking
> 
> More locking than the drm_modeset_lock_all() above the switch? Ouch. :)

Oh, I didn't spot that one. Well, that one should be removed imo, we dont
want any vblank wait path to hold these locks, it looks bad. Otoh we still
need to grab the dev->mode_config.mutex, because that's also the fbdev
lock. There's a plan somewhere to give the fbdev helpers their own lock,
so switching to just mode_config.mutex + the changes I suggestd would be
good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1578013 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

Hi,

On 9 February 2017 at 17:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
>> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
>> +{
>> +     struct drm_fb_helper *fb_helper = info->par;
>> +     struct drm_device *dev = fb_helper->dev;
>> +     unsigned int i;
>> +     int ret = 0;
>> +
>> +     drm_modeset_lock_all(dev);
>> +     if (!drm_fb_helper_is_bound(fb_helper)) {
>> +             drm_modeset_unlock_all(dev);
>> +             return -EBUSY;
>> +     }
>> +
>> +     switch (cmd) {
>> +     case FBIO_WAITFORVSYNC:
>> +             for (i = 0; i < fb_helper->crtc_count; i++) {
>> +                     struct drm_mode_set *mode_set;
>> +                     struct drm_crtc *crtc;
>> +
>> +                     mode_set = &fb_helper->crtc_info[i].mode_set;
>> +                     crtc = mode_set->crtc;
>> +
>> +                     /*
>> +                      * Only call drm_crtc_wait_one_vblank for crtcs that
>> +                      * are currently enabled.
>> +                      */
>> +                     if (!crtc->enabled)
>> +                             continue;
>
> This needs locking

More locking than the drm_modeset_lock_all() above the switch? Ouch. :)

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1578496 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> From: Stefan Christ <s.christ@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 | 55 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     |  5 ++-
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e934b541feea..39a3532e311c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
>  
>  /**
> + * drm_fb_helper_ioctl - legacy ioctl implementation
> + * @info: fbdev registered by the helper
> + * @cmd: ioctl command
> + * @arg: ioctl argument
> + *
> + * A helper to implement the standard fbdev ioctl. Only
> + * FBIO_WAITFORVSYNC is implemented for now.
> + */
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_device *dev = fb_helper->dev;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	drm_modeset_lock_all(dev);
> +	if (!drm_fb_helper_is_bound(fb_helper)) {
> +		drm_modeset_unlock_all(dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (cmd) {
> +	case FBIO_WAITFORVSYNC:
> +		for (i = 0; i < fb_helper->crtc_count; i++) {

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.

> +			struct drm_mode_set *mode_set;
> +			struct drm_crtc *crtc;
> +
> +			mode_set = &fb_helper->crtc_info[i].mode_set;
> +			crtc = mode_set->crtc;
> +
> +			/*
> +			 * Only call drm_crtc_wait_one_vblank for crtcs that
> +			 * are currently enabled.
> +			 */
> +			if (!crtc->enabled)
> +				continue;
> +
> +			ret = drm_crtc_vblank_get(crtc);
> +			if (!ret) {
> +				drm_crtc_wait_one_vblank(crtc);
> +				drm_crtc_vblank_put(crtc);
> +			}

This looks quite sub-optimal. It should rather do something along the
lines of what drm_atomic_helper_wait_for_vblanks() does.

> +		}
> +		goto unlock;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +unlock:
> +	drm_modeset_unlock_all(dev);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_ioctl);
> +
> +/**
>   * drm_fb_helper_check_var - implementation for ->fb_check_var
>   * @var: screeninfo to check
>   * @info: fbdev registered by the helper
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 975deedd593e..460be9d9fa98 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -230,7 +230,8 @@ struct drm_fb_helper {
>  	.fb_blank	= drm_fb_helper_blank, \
>  	.fb_pan_display	= drm_fb_helper_pan_display, \
>  	.fb_debug_enter = drm_fb_helper_debug_enter, \
> -	.fb_debug_leave = drm_fb_helper_debug_leave
> +	.fb_debug_leave = drm_fb_helper_debug_leave, \
> +	.fb_ioctl	= drm_fb_helper_ioctl
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
> @@ -286,6 +287,8 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
>  
>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
>  
> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg);
> +
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel);
>  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper);
> -- 
> git-series 0.8.11
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1579591 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

[Multipart message — attachments visible in raw view] — view raw

Hi Ville,

On Fri, Feb 10, 2017 at 04:06:05PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> > 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 | 55 ++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_fb_helper.h     |  5 ++-
> >  2 files changed, 59 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index e934b541feea..39a3532e311c 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> >  EXPORT_SYMBOL(drm_fb_helper_setcmap);
> >  
> >  /**
> > + * drm_fb_helper_ioctl - legacy ioctl implementation
> > + * @info: fbdev registered by the helper
> > + * @cmd: ioctl command
> > + * @arg: ioctl argument
> > + *
> > + * A helper to implement the standard fbdev ioctl. Only
> > + * FBIO_WAITFORVSYNC is implemented for now.
> > + */
> > +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct drm_device *dev = fb_helper->dev;
> > +	unsigned int i;
> > +	int ret = 0;
> > +
> > +	drm_modeset_lock_all(dev);
> > +	if (!drm_fb_helper_is_bound(fb_helper)) {
> > +		drm_modeset_unlock_all(dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (cmd) {
> > +	case FBIO_WAITFORVSYNC:
> > +		for (i = 0; i < fb_helper->crtc_count; i++) {
> 
> 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.

I guess I could just use that index to retrieve only the right CRTC in
fb_helper->crtc_info.

> 
> > +			struct drm_mode_set *mode_set;
> > +			struct drm_crtc *crtc;
> > +
> > +			mode_set = &fb_helper->crtc_info[i].mode_set;
> > +			crtc = mode_set->crtc;
> > +
> > +			/*
> > +			 * Only call drm_crtc_wait_one_vblank for crtcs that
> > +			 * are currently enabled.
> > +			 */
> > +			if (!crtc->enabled)
> > +				continue;
> > +
> > +			ret = drm_crtc_vblank_get(crtc);
> > +			if (!ret) {
> > +				drm_crtc_wait_one_vblank(crtc);
> > +				drm_crtc_vblank_put(crtc);
> > +			}
> 
> This looks quite sub-optimal. It should rather do something along the
> lines of what drm_atomic_helper_wait_for_vblanks() does.

How is that suboptimal?

As far as I can see, they are both doing the same thing (except one
does for all the CRTCs, but I guess that would no longer be needed
with the change you suggested above):
  - drm_crtc_vblank_get
  - Wait for the vblank count to change
  - drm_crtc_vblank_put

Maxime

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

[toc] | [prev] | [next] | [standalone]

#1579832 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

On Mon, Feb 13, 2017 at 11:35:18AM +0100, Maxime Ripard wrote:
> Hi Ville,
> 
> On Fri, Feb 10, 2017 at 04:06:05PM +0200, Ville Syrjälä wrote:
> > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> > > 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 | 55 ++++++++++++++++++++++++++++++++++-
> > >  include/drm/drm_fb_helper.h     |  5 ++-
> > >  2 files changed, 59 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index e934b541feea..39a3532e311c 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> > >  EXPORT_SYMBOL(drm_fb_helper_setcmap);
> > >  
> > >  /**
> > > + * drm_fb_helper_ioctl - legacy ioctl implementation
> > > + * @info: fbdev registered by the helper
> > > + * @cmd: ioctl command
> > > + * @arg: ioctl argument
> > > + *
> > > + * A helper to implement the standard fbdev ioctl. Only
> > > + * FBIO_WAITFORVSYNC is implemented for now.
> > > + */
> > > +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> > > +{
> > > +	struct drm_fb_helper *fb_helper = info->par;
> > > +	struct drm_device *dev = fb_helper->dev;
> > > +	unsigned int i;
> > > +	int ret = 0;
> > > +
> > > +	drm_modeset_lock_all(dev);
> > > +	if (!drm_fb_helper_is_bound(fb_helper)) {
> > > +		drm_modeset_unlock_all(dev);
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	switch (cmd) {
> > > +	case FBIO_WAITFORVSYNC:
> > > +		for (i = 0; i < fb_helper->crtc_count; i++) {
> > 
> > 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.
> 
> I guess I could just use that index to retrieve only the right CRTC in
> fb_helper->crtc_info.
> 
> > 
> > > +			struct drm_mode_set *mode_set;
> > > +			struct drm_crtc *crtc;
> > > +
> > > +			mode_set = &fb_helper->crtc_info[i].mode_set;
> > > +			crtc = mode_set->crtc;
> > > +
> > > +			/*
> > > +			 * Only call drm_crtc_wait_one_vblank for crtcs that
> > > +			 * are currently enabled.
> > > +			 */
> > > +			if (!crtc->enabled)
> > > +				continue;
> > > +
> > > +			ret = drm_crtc_vblank_get(crtc);
> > > +			if (!ret) {
> > > +				drm_crtc_wait_one_vblank(crtc);
> > > +				drm_crtc_vblank_put(crtc);
> > > +			}
> > 
> > This looks quite sub-optimal. It should rather do something along the
> > lines of what drm_atomic_helper_wait_for_vblanks() does.
> 
> How is that suboptimal?

You're serializing the waits rather than doing them in parallel.

Let's look at a simple three crtc example (|=vblank):

        time -->
CRTC 1:     |     |     |     |
CRTC 2:   |     |     |     |
CRTC 3: |     |     |     |

Your code waits until here:
            |   |   |
            1   2   3
                    ^

Optimal code would wait until here:
            |
            ^
 
-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1581327 — Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Mon, Feb 13, 2017 at 04:45:33PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 13, 2017 at 11:35:18AM +0100, Maxime Ripard wrote:
> > Hi Ville,
> > 
> > On Fri, Feb 10, 2017 at 04:06:05PM +0200, Ville Syrjälä wrote:
> > > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote:
> > > > 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 | 55 ++++++++++++++++++++++++++++++++++-
> > > >  include/drm/drm_fb_helper.h     |  5 ++-
> > > >  2 files changed, 59 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > index e934b541feea..39a3532e311c 100644
> > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> > > >  EXPORT_SYMBOL(drm_fb_helper_setcmap);
> > > >  
> > > >  /**
> > > > + * drm_fb_helper_ioctl - legacy ioctl implementation
> > > > + * @info: fbdev registered by the helper
> > > > + * @cmd: ioctl command
> > > > + * @arg: ioctl argument
> > > > + *
> > > > + * A helper to implement the standard fbdev ioctl. Only
> > > > + * FBIO_WAITFORVSYNC is implemented for now.
> > > > + */
> > > > +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
> > > > +{
> > > > +	struct drm_fb_helper *fb_helper = info->par;
> > > > +	struct drm_device *dev = fb_helper->dev;
> > > > +	unsigned int i;
> > > > +	int ret = 0;
> > > > +
> > > > +	drm_modeset_lock_all(dev);
> > > > +	if (!drm_fb_helper_is_bound(fb_helper)) {
> > > > +		drm_modeset_unlock_all(dev);
> > > > +		return -EBUSY;
> > > > +	}
> > > > +
> > > > +	switch (cmd) {
> > > > +	case FBIO_WAITFORVSYNC:
> > > > +		for (i = 0; i < fb_helper->crtc_count; i++) {
> > > 
> > > 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.
> > 
> > I guess I could just use that index to retrieve only the right CRTC in
> > fb_helper->crtc_info.
> > 
> > > 
> > > > +			struct drm_mode_set *mode_set;
> > > > +			struct drm_crtc *crtc;
> > > > +
> > > > +			mode_set = &fb_helper->crtc_info[i].mode_set;
> > > > +			crtc = mode_set->crtc;
> > > > +
> > > > +			/*
> > > > +			 * Only call drm_crtc_wait_one_vblank for crtcs that
> > > > +			 * are currently enabled.
> > > > +			 */
> > > > +			if (!crtc->enabled)
> > > > +				continue;
> > > > +
> > > > +			ret = drm_crtc_vblank_get(crtc);
> > > > +			if (!ret) {
> > > > +				drm_crtc_wait_one_vblank(crtc);
> > > > +				drm_crtc_vblank_put(crtc);
> > > > +			}
> > > 
> > > This looks quite sub-optimal. It should rather do something along the
> > > lines of what drm_atomic_helper_wait_for_vblanks() does.
> > 
> > How is that suboptimal?
> 
> You're serializing the waits rather than doing them in parallel.
> 
> Let's look at a simple three crtc example (|=vblank):
> 
>         time -->
> CRTC 1:     |     |     |     |
> CRTC 2:   |     |     |     |
> CRTC 3: |     |     |     |
> 
> Your code waits until here:
>             |   |   |
>             1   2   3
>                     ^
> 
> Optimal code would wait until here:
>             |
>             ^

Ah, right. But then, this would happen only if we loop over all the
CRTCs, which shouldn't happen in the v2 as you suggested.

Out of curiosity, how could we fix this if we were to loop over all
the framebuffers? By waiting on any vblank count to change?

Thanks,
Maxime

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web