| Message ID | a71d7325494bef185d015697cbc9cbcdf37a923f.1487175046.git-series.maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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
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
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) { }