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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)