[PATCH 1/2] drm: Fix vblank refcount during modeset

[PATCH 1/2] drm: Fix vblank refcount during modeset

[Yunxiang Li]

drm_crtc_vblank_off increments the refcount to prevent vblank from
getting enabled during modeset, but this causes the refcount elsewhere
to be off if they call drm_vblank_get without checking the return and
do a drm_vblank_put later. This can be reproduced by toggling vrr mode
on amdgpu.

Since drm_crtc_vblank_on later re-enables vblank if the refcount is not
zero, letting drm_vblank_get succeed during modeset should fix the behavior.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/1380
Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
---
 drivers/gpu/drm/drm_vblank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 2ff31717a3de..159d13b5d97b 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1174,7 +1174,7 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	if (atomic_add_return(1, &vblank->refcount) == 1) {
 		ret = drm_vblank_enable(dev, pipe);
 	} else {
-		if (!vblank->enabled) {
+		if (!vblank->enabled && !vblank->inmodeset) {
 			atomic_dec(&vblank->refcount);
 			ret = -EINVAL;
 		}
-- 
2.37.1

[PATCH 2/2] drm: get lock before accessing vblank refcount

[Yunxiang Li]

Acquire vbl_lock before accessing vblank refcount in drm_vblank_put,
just like everywhere else that access the refcount.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
---
 drivers/gpu/drm/drm_vblank.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 159d13b5d97b..77b8c40fc7ba 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1203,15 +1203,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_get);
 void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+	unsigned long irqflags;
+	int ret;
 
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
 
-	if (drm_WARN_ON(dev, atomic_read(&vblank->refcount) == 0))
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	if (drm_WARN_ON(dev, atomic_read(&vblank->refcount) == 0)) {
+		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 		return;
+	}
 
 	/* Last user schedules interrupt disable */
-	if (atomic_dec_and_test(&vblank->refcount)) {
+	ret = atomic_dec_and_test(&vblank->refcount);
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+	if (ret) {
 		if (drm_vblank_offdelay == 0)
 			return;
 		else if (drm_vblank_offdelay < 0)
-- 
2.37.1

Re: [PATCH 1/2] drm: Fix vblank refcount during modeset

[Li, Yunxiang (Teddy)]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

[AMD Official Use Only - General]

I found out that elsewhere in the drm code (e.g. in drm_atomic_helper.c) expects drm_vblank_get() to fail as part of normal operation. So this patch doesn't seem appropriate anymore and it seems more appropriate to hunt down all the unchecked calls for drm_vblank_get and fix them instead. I don't know how to best make sure this doesn't reoccur in the future though.

Regards,
Yunxiang


[-- Attachment #2: Type: text/html, Size: 1423 bytes --]

Re: [PATCH 2/2] drm: get lock before accessing vblank refcount

[Daniel Vetter]

On Fri, Jul 22, 2022 at 05:52:34PM -0400, Yunxiang Li wrote:
> Acquire vbl_lock before accessing vblank refcount in drm_vblank_put,
> just like everywhere else that access the refcount.
> 
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>

The entire point of using atomic for the refcount is that we can check it
lockless, so I'm not sure what you're trying to fix here?

For the first patch I think it's clear that the bug needs to be fixed in
amdgpu dc code already.
-Daniel
> ---
>  drivers/gpu/drm/drm_vblank.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 159d13b5d97b..77b8c40fc7ba 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1203,15 +1203,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_get);
>  void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	unsigned long irqflags;
> +	int ret;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
>  
> -	if (drm_WARN_ON(dev, atomic_read(&vblank->refcount) == 0))
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	if (drm_WARN_ON(dev, atomic_read(&vblank->refcount) == 0)) {
> +		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  		return;
> +	}
>  
>  	/* Last user schedules interrupt disable */
> -	if (atomic_dec_and_test(&vblank->refcount)) {
> +	ret = atomic_dec_and_test(&vblank->refcount);
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +	if (ret) {
>  		if (drm_vblank_offdelay == 0)
>  			return;
>  		else if (drm_vblank_offdelay < 0)
> -- 
> 2.37.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

RE: [PATCH 2/2] drm: get lock before accessing vblank refcount

[Li, Yunxiang (Teddy)]

[Public]

Hi Daniel,

I added the check because I saw that the refcount was always updated/read with lock held elsewhere, and the pattern looked very similar to in the put e.g. drm_crtc_vblank_reset. This patchset is outdated by now and I've sent a fix to amd-gfx for the specific issue in amdgpu.

However, I think the way drm_crtc_vblank_on/off functions increments the refcount without enabling the vblank is still a bit risky given how many unchecked calls to drm_vblank_get there is elsewhere. Maybe it's more appropriate to simply add an WARN to drm_vblank_get when it's called with inmodeset set? This way the WARN happens right at the problematic point, instead of far into the future when the put is called.

Yunxiang

Re: [PATCH 2/2] drm: get lock before accessing vblank refcount

[Daniel Vetter]

On Tue, Sep 06, 2022 at 08:18:30PM +0000, Li, Yunxiang (Teddy) wrote:
> [Public]
> 
> Hi Daniel,
> 
> I added the check because I saw that the refcount was always
> updated/read with lock held elsewhere, and the pattern looked very
> similar to in the put e.g. drm_crtc_vblank_reset. This patchset is
> outdated by now and I've sent a fix to amd-gfx for the specific issue in
> amdgpu.
> 
> However, I think the way drm_crtc_vblank_on/off functions increments the
> refcount without enabling the vblank is still a bit risky given how many
> unchecked calls to drm_vblank_get there is elsewhere. Maybe it's more
> appropriate to simply add an WARN to drm_vblank_get when it's called
> with inmodeset set? This way the WARN happens right at the problematic
> point, instead of far into the future when the put is called.

drm_crtc_vblank_get failing when the crtc is off is how this is supposed
to work, calling WARN_ON or similar in there would upset everything.

What might be an option is adding __must_check or similar annotations, but
the problem is that in many cases the driver knows that it cannot fail, so
this isn't great either.

Another option would be to split this up into drm_crtc_vblank_get with
void return value (and a WARN_ON when it fails), and
drm_crtc_vblank_try_get, which can fail. And then go through _all_ the
callers and audit them.

Imo not really worth the work, but we could do that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch