Re: [PATCH] drm/client: check whether CRTC is active before waiting for vblank

[Icenowy Zheng]

在 2026-05-22五的 15:24 +0200，Thomas Zimmermann写道：
> Hi
> 
> Am 22.05.26 um 15:13 schrieb Ville Syrjälä:
> > On Fri, May 22, 2026 at 01:55:59PM +0200, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 19.05.26 um 11:24 schrieb Icenowy Zheng:
> > > > Currently the implementaion of
> > > > drm_client_modeset_wait_for_vblank()
> > > > assumes drm_vblank_get() will fail when the CRTC isn't active.
> > > > However
> > > > it seems that this is not true, and running fbcon on a device
> > > > with the
> > > > first CRTC inactive will lead to kernel warning in some cases
> > > > (which
> > > > could be reproduced with the loongson driver).
> > > > 
> > > > Change the implementation to add a check for the active state
> > > > (atomic) /
> > > > enabled state (non-atomic) before calling drm_vblank_get(). As
> > > > the
> > > > assumption of drm_vblank_get() failing for inactive CRTC isn't
> > > > met, the
> > > > error status of drm_vblank_get() can now be exported too.
> > > > 
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker
> > > > with vblank")
> > > > Signed-off-by: Icenowy Zheng <zhengxingda@xxxxxxxxxxx>
> > > > ---
> > > >    drivers/gpu/drm/drm_client_modeset.c | 13 +++++++++++--
> > > >    1 file changed, 11 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c
> > > > b/drivers/gpu/drm/drm_client_modeset.c
> > > > index bb49b8361271a..1b03bf351256e 100644
> > > > --- a/drivers/gpu/drm/drm_client_modeset.c
> > > > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > > > @@ -1310,7 +1310,7 @@ int
> > > > drm_client_modeset_wait_for_vblank(struct drm_client_dev
> > > > *client, unsigned i
> > > >    {
> > > >    	struct drm_device *dev = client->dev;
> > > >    	struct drm_crtc *crtc;
> > > > -	int ret;
> > > > +	int ret = 0;
> > > >    
> > > >    	/*
> > > >    	 * Rate-limit update frequency to vblank. If there's a
> > > > DRM master
> > > > @@ -1326,15 +1326,24 @@ int
> > > > drm_client_modeset_wait_for_vblank(struct drm_client_dev
> > > > *client, unsigned i
> > > >    	 * Only wait for a vblank event if the CRTC is
> > > > enabled, otherwise
> > > >    	 * just don't do anything, not even report an error.
> > > >    	 */
> > > > +	if (drm_drv_uses_atomic_modeset(dev)) {
> > > > +		if (!crtc->state || !crtc->state->active)
> > > > +			goto out;
> > > > +	} else {
> > > > +		if (!crtc->enabled)
> > > > +			goto out;
> > > > +	}
> > > > +
> > > This part is good.
> > Locking is missing.
> 
> Ok
> 
> > 
> > > >    	ret = drm_crtc_vblank_get(crtc);
> > > >    	if (!ret) {
> > > >    		drm_crtc_wait_one_vblank(crtc);
> > > >    		drm_crtc_vblank_put(crtc);
> > > >    	}
> > > >    
> > > > +out:
> > > >    	drm_master_internal_release(dev);
> > > >    
> > > > -	return 0;
> > > > +	return ret;
> > > But this isn't. There can be CRTCs without any vblank at all. We
> > > still
> > > want to fail silently for them. So we still have to return 0
> > > here.
> > > 
> > > Having set this, fixing this helper is only partially what you
> > > want.
> > > Since your device has vblanking, the emulation should check on
> > > the
> > > correct CRTC. IOW you need to pass the right CRTC index at
> > > 
> > > https://elixir.bootlin.com/linux/v7.1-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L237
> > > https://elixir.bootlin.com/linux/v7.1-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L920
> > > 
> > > I'm not quite sure how to support this. The CRTC is under
> > > fb_helper->client.modesets.crtc. You'd have to figure out which
> > > is the
> > > relevant one and use that. But that's also not so great, as fbdev
> > > ioctls
> > > only support CRTC 0. Doing internal re-mappings only complicates
> > > matters.
> > > 
> > > But why does your HW use CRTC 1 in the first place.
> > Could be eg. the enabled outputs can't be driven with CRTC 0.
> > 
> > I guess what you want to do is pick the first crtc from modesets[]
> > which is enabled. Or perhaps even "pick the Nth enabled crtc from
> > modesets[] based on the ioctl argument".
> 
> The enable-status of each CRTC could change later on, which might
> lead 

Well, maybe it's too difficult for things like fbdev to support
hotplugging outputs.

Maybe just use crtcs from modeset[] and check its enable status before
doing vblank wait, and have the vsync waiting falling back to nothing
when the initial CRTC of it isn't now enabled?

This will at least make some usages of fbdev work and restrict the
failure for other usages (e.g. doesn't enable vblank on a not enabled
CRTC).

Thanks,
Icenowy

> to problems.  Picking the one CRTC/output with the lowest spec and 
> mirroring it to the others might work. This CRTC would then be the
> one 
> to wait for.
> 
> Best regards
> Thomas
> 
> >