| Message ID | b10828a704107b4825762c3b7419bd82a5b59147.1486031436.git-series.maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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: | ^
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
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);