Re: [PATCH] drm/vblank: Require a driver register vblank support for 0 or all CRTCs

[Louis Chauvet]

Le 27/09/24 - 16:39, Lyude Paul a écrit :
> Currently, there's nothing actually stopping a driver from only registering
> vblank support for some of it's CRTCs and not for others. As far as I can
> tell, this isn't really defined behavior on the C side of things - as the
> documentation explicitly mentions to not use drm_vblank_init() if you don't
> have vblank support - since DRM then steps in and adds its own vblank
> emulation implementation.
> 
> So, let's fix this edge case and check to make sure it's all or none.
> 
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> Fixes: 3ed4351a83ca ("drm: Extract drm_vblank.[hc]")
> Cc: Stefan Agner <stefan@xxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxxx>
> Cc: Simona Vetter <simona@xxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.13+
> ---
>  drivers/gpu/drm/drm_vblank.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 94e45ed6869d0..4d00937e8ca2e 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -525,9 +525,19 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
>   */
>  int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
>  {
> +	struct drm_crtc *crtc;
>  	int ret;
>  	unsigned int i;
>  
> +	// Confirm that the required vblank functions have been filled out for all CRTCS
> +	drm_for_each_crtc(crtc, dev) {
> +		if (!crtc->funcs->enable_vblank || !crtc->funcs->disable_vblank) {
> +			drm_err(dev, "CRTC vblank functions not initialized for %s, abort\n",
> +				crtc->name);
> +			return -EINVAL;
> +		}
> +	}
> +

Hi,

I noticed that the kernel bot reported an issue with VKMS and this patch.

I did not take the time to reproduce the issue, but it may come from the 
fact that VKMS call drm_vblank_init before calling 
drmm_crtc_init_with_planes [1]. I don't see anything in the documentation 
that requires the CRTC to be initialized before the vblank, is it a change 
of the API or an issue in VKMS since 2018 [2]?

Anyway, if this is a requirement, can you explain it in the 
drm_vblank_init documentation?

Thanks,
Louis Chauvet

[1]:https://elixir.bootlin.com/linux/v6.11.2/source/drivers/gpu/drm/vkms/vkms_drv.c#L209
[2]:https://lore.kernel.org/all/5d9ca7b3884c1995bd4a983b1d2ff1b840eb7f1a.1531402095.git.rodrigosiqueiramelo@xxxxxxxxx/

>  	spin_lock_init(&dev->vbl_lock);
>  	spin_lock_init(&dev->vblank_time_lock);
>  
> 
> base-commit: 22512c3ee0f47faab5def71c4453638923c62522
> -- 
> 2.46.1
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com