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

[Ville Syrjälä]

On Fri, May 22, 2026 at 03:24:05PM +0200, Thomas Zimmermann wrote:
> 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 
> to problems.

Sound like a locking issue if someone is changing the configuration
at the same time we're trying to do the vblank wait here.

> 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
> 
> >
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
> 

-- 
Ville Syrjälä
Intel