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

[Maxime Ripard]

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@xxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxx>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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

Attachment: signature.asc
Description: PGP signature