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: [email protected]
> > Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
> > Signed-off-by: Icenowy Zheng <[email protected]>
> > ---
> > 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.
>
> > 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".
--
Ville Syrjälä
Intel
