drm/client: check whether CRTC is active before waiting for vblank

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

Posted by Icenowy Zheng 5 days, 17 hours ago

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@vger.kernel.org
Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 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;
+	}
+
 	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;
 }
 EXPORT_SYMBOL(drm_client_modeset_wait_for_vblank);
 
-- 
2.52.0

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

Posted by Thomas Zimmermann 2 days, 15 hours ago

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@vger.kernel.org
> Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> ---
>   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.

>   	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.

Best regards
Thomas



>   }
>   EXPORT_SYMBOL(drm_client_modeset_wait_for_vblank);
>   

-- 
--
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)

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

Posted by Ville Syrjälä 2 days, 13 hours ago

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@vger.kernel.org
> > Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
> > Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> > ---
> >   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

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

Posted by Thomas Zimmermann 2 days, 13 hours ago

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@vger.kernel.org
>>> Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
>>> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
>>> ---
>>>    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.  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)

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

Posted by Ville Syrjälä 2 days, 13 hours ago

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@vger.kernel.org
> >>> Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
> >>> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> >>> ---
> >>>    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

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

Posted by Jani Nikula 5 days, 17 hours ago

On Tue, 19 May 2026, Icenowy Zheng <zhengxingda@iscas.ac.cn> wrote:
> 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@vger.kernel.org
> Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with vblank")
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> ---
>  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.
>  	 */

I'll dodge the question whether the change below is right or not, but
for sure the comment above needs to be amended to match the change.

(Please wait for other review comments before sending another version
with the comment changed.)

BR,
Jani.

> +	if (drm_drv_uses_atomic_modeset(dev)) {
> +		if (!crtc->state || !crtc->state->active)
> +			goto out;
> +	} else {
> +		if (!crtc->enabled)
> +			goto out;
> +	}
> +
>  	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;
>  }
>  EXPORT_SYMBOL(drm_client_modeset_wait_for_vblank);

-- 
Jani Nikula, Intel

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

Posted by Icenowy Zheng 5 days, 15 hours ago

在 2026-05-19二的 12:41 +0300，Jani Nikula写道：
> On Tue, 19 May 2026, Icenowy Zheng <zhengxingda@iscas.ac.cn> wrote:
> > 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@vger.kernel.org
> > Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with
> > vblank")
> > Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> > ---
> >  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.
> >  	 */
> 
> I'll dodge the question whether the change below is right or not, but
> for sure the comment above needs to be amended to match the change.

If the change is right, it perfectly matches what the comment above is
saying -- it's the current behavior that does not match the comment.

Thanks,
Icenowy

> 
> (Please wait for other review comments before sending another version
> with the comment changed.)
> 
> BR,
> Jani.
> 
> > +	if (drm_drv_uses_atomic_modeset(dev)) {
> > +		if (!crtc->state || !crtc->state->active)
> > +			goto out;
> > +	} else {
> > +		if (!crtc->enabled)
> > +			goto out;
> > +	}
> > +
> >  	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;
> >  }
> >  EXPORT_SYMBOL(drm_client_modeset_wait_for_vblank);

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

Posted by Maarten Lankhorst 2 days, 17 hours ago

Hey,

Den 2026-05-19 kl. 13:29, skrev Icenowy Zheng:
> 在 2026-05-19二的 12:41 +0300，Jani Nikula写道：
>> On Tue, 19 May 2026, Icenowy Zheng <zhengxingda@iscas.ac.cn> wrote:
>>> 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@vger.kernel.org
>>> Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker with
>>> vblank")
>>> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
>>> ---
>>>  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.
>>>  	 */
>>
>> I'll dodge the question whether the change below is right or not, but
>> for sure the comment above needs to be amended to match the change.
> 
> If the change is right, it perfectly matches what the comment above is
> saying -- it's the current behavior that does not match the comment.
> 
> Thanks,
> Icenowy
I would rather have expected drm_fb_helper_ioctl to fail like you mention.
Probably needs a fbcon_is_active() there to prevent it.

The damage helper should not be triggered if no CRTC is active, so that means
the check here is slightly too late.

Can you fix it at a different level, like damage helper or its callers instead?

I believe when the client gets suspended, all the pending damage is flushed before
suspend.

Kind regards,
~Maarten Lankhorst

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

Posted by Icenowy Zheng 2 days, 17 hours ago

在 2026-05-22五的 11:23 +0200，Maarten Lankhorst写道：
> Hey,
> 
> Den 2026-05-19 kl. 13:29, skrev Icenowy Zheng:
> > 在 2026-05-19二的 12:41 +0300，Jani Nikula写道：
> > > On Tue, 19 May 2026, Icenowy Zheng <zhengxingda@iscas.ac.cn>
> > > wrote:
> > > > 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@vger.kernel.org
> > > > Fixes: d8c4bddcd8bc ("drm/fb-helper: Synchronize dirty worker
> > > > with
> > > > vblank")
> > > > Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> > > > ---
> > > >  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.
> > > >  	 */
> > > 
> > > I'll dodge the question whether the change below is right or not,
> > > but
> > > for sure the comment above needs to be amended to match the
> > > change.
> > 
> > If the change is right, it perfectly matches what the comment above
> > is
> > saying -- it's the current behavior that does not match the
> > comment.
> > 
> > Thanks,
> > Icenowy
> I would rather have expected drm_fb_helper_ioctl to fail like you
> mention.
> Probably needs a fbcon_is_active() there to prevent it.
> 
> The damage helper should not be triggered if no CRTC is active, so
> that means
> the check here is slightly too late.

Well the problem here seems to be a CRTC is active, but it's not CRTC 0
(for embedded hardwares CRTCs could be somehow fixed and tied to
physical ports); and the fbdev helpers hardcode 0 as the CRTC ID.

Thanks,
Icenowy

> 
> Can you fix it at a different level, like damage helper or its
> callers instead?
> 
> I believe when the client gets suspended, all the pending damage is
> flushed before
> suspend.
> 
> Kind regards,
> ~Maarten Lankhorst