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@redhat.com>
Fixes: 3ed4351a83ca ("drm: Extract drm_vblank.[hc]")
Cc: Stefan Agner <stefan@agner.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # 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;
+		}
+	}
+
 	spin_lock_init(&dev->vbl_lock);
 	spin_lock_init(&dev->vblank_time_lock);
 

base-commit: 22512c3ee0f47faab5def71c4453638923c62522
-- 
2.46.1

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@redhat.com>
> Fixes: 3ed4351a83ca ("drm: Extract drm_vblank.[hc]")
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # 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@gmail.com/

>  	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