[PATCH 0/2] hibmc: Improve vblank handling

[PATCH 0/2] hibmc: Improve vblank handling

[Thomas Zimmermann]

Vblank event handling in hibmc is currently somewhat disconnected
from what the hardware does. Patch 1 updates IRQ handlers to signal
whether the driver handled an IRQ or not. Patch 2 connects hibmc's
vblank events to the hardware's vblank interrupts.

Tested on an RH1288 v3 system.

Thomas Zimmermann (2):
  drm/hibmc: Return IRQ_NONE for unhandled interrupts
  drm/hibmc: Set up vblank event to be send on the next vblank

 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 16 ++--------------
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c |  5 +++--
 2 files changed, 5 insertions(+), 16 deletions(-)


base-commit: dc462ab791b686c48545c160ecc81be64f77a846
-- 
2.55.0

[PATCH 1/2] drm/hibmc: Return IRQ_NONE for unhandled interrupts

[Thomas Zimmermann]

Let the kernel know if hibmc did not handle an interrupt. Allows
the kernel to detect spurious interrupts and maintain diagnostics.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 4d85c89f3f88..602930964e93 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -49,9 +49,10 @@ static irqreturn_t hibmc_interrupt(int irq, void *arg)
 		writel(HIBMC_RAW_INTERRUPT_VBLANK(1),
 		       priv->mmio + HIBMC_RAW_INTERRUPT);
 		drm_handle_vblank(dev, 0);
+		return IRQ_HANDLED;
 	}
 
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
@@ -67,7 +68,7 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
 		return IRQ_WAKE_THREAD;
 	}
 
-	return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
-- 
2.55.0

[PATCH 2/2] drm/hibmc: Set up vblank event to be send on the next vblank

[Thomas Zimmermann]

Do not send the vblank event when the page flip completes, but when
the vblank interrupt fires. Sending the vblank event at the wrong time
can break synchronization of the display update with the vblank period.

Use the vblank helper drm_crtc_vblank_atomic_flush(), which sets up
the vblank event accordingly. In case of an error, it still sends out
the vblank event immediately; thus not starving consumers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
index b4ab53db1c08..72ce696e6d11 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
@@ -20,6 +20,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_vblank_helper.h>
 
 #include "hibmc_drm_drv.h"
 #include "hibmc_drm_regs.h"
@@ -420,19 +421,6 @@ static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
 	/* We can add more initialization as needed. */
 }
 
-static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
-				    struct drm_atomic_commit *state)
-
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&crtc->dev->event_lock, flags);
-	if (crtc->state->event)
-		drm_crtc_send_vblank_event(crtc, crtc->state->event);
-	crtc->state->event = NULL;
-	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-}
-
 static int hibmc_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct hibmc_drm_private *priv = to_hibmc_drm_private(crtc->dev);
@@ -502,7 +490,7 @@ static const struct drm_crtc_funcs hibmc_crtc_funcs = {
 static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
 	.mode_set_nofb	= hibmc_crtc_mode_set_nofb,
 	.atomic_begin	= hibmc_crtc_atomic_begin,
-	.atomic_flush	= hibmc_crtc_atomic_flush,
+	.atomic_flush	= drm_crtc_vblank_atomic_flush,
 	.atomic_enable	= hibmc_crtc_atomic_enable,
 	.atomic_disable	= hibmc_crtc_atomic_disable,
 	.mode_valid = hibmc_crtc_mode_valid,
-- 
2.55.0

Re: [PATCH 1/2] drm/hibmc: Return IRQ_NONE for unhandled interrupts

[Yongbang Shi]

> Let the kernel know if hibmc did not handle an interrupt. Allows
> the kernel to detect spurious interrupts and maintain diagnostics.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 4d85c89f3f88..602930964e93 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -49,9 +49,10 @@ static irqreturn_t hibmc_interrupt(int irq, void *arg)
>   		writel(HIBMC_RAW_INTERRUPT_VBLANK(1),
>   		       priv->mmio + HIBMC_RAW_INTERRUPT);
>   		drm_handle_vblank(dev, 0);
> +		return IRQ_HANDLED;
>   	}
>   
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;
>   }
>   
>   static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
> @@ -67,7 +68,7 @@ static irqreturn_t hibmc_dp_interrupt(int irq, void *arg)
>   		return IRQ_WAKE_THREAD;
>   	}
>   
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;
>   }
>   
>   static int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,

Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>

Re: [PATCH 2/2] drm/hibmc: Set up vblank event to be send on the next vblank

[Yongbang Shi]

> Do not send the vblank event when the page flip completes, but when
> the vblank interrupt fires. Sending the vblank event at the wrong time
> can break synchronization of the display update with the vblank period.
> 
> Use the vblank helper drm_crtc_vblank_atomic_flush(), which sets up
> the vblank event accordingly. In case of an error, it still sends out
> the vblank event immediately; thus not starving consumers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 16 ++--------------
>   1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index b4ab53db1c08..72ce696e6d11 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -20,6 +20,7 @@
>   #include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_vblank.h>
> +#include <drm/drm_vblank_helper.h>
>   
>   #include "hibmc_drm_drv.h"
>   #include "hibmc_drm_regs.h"
> @@ -420,19 +421,6 @@ static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
>   	/* We can add more initialization as needed. */
>   }
>   
> -static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
> -				    struct drm_atomic_commit *state)
> -
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&crtc->dev->event_lock, flags);
> -	if (crtc->state->event)
> -		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> -	crtc->state->event = NULL;
> -	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -}
> -
>   static int hibmc_crtc_enable_vblank(struct drm_crtc *crtc)
>   {
>   	struct hibmc_drm_private *priv = to_hibmc_drm_private(crtc->dev);
> @@ -502,7 +490,7 @@ static const struct drm_crtc_funcs hibmc_crtc_funcs = {
>   static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>   	.mode_set_nofb	= hibmc_crtc_mode_set_nofb,
>   	.atomic_begin	= hibmc_crtc_atomic_begin,
> -	.atomic_flush	= hibmc_crtc_atomic_flush,
> +	.atomic_flush	= drm_crtc_vblank_atomic_flush,
>   	.atomic_enable	= hibmc_crtc_atomic_enable,
>   	.atomic_disable	= hibmc_crtc_atomic_disable,
>   	.mode_valid = hibmc_crtc_mode_valid,

Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>

Re: [PATCH 2/2] drm/hibmc: Set up vblank event to be send on the next vblank

[Thomas Zimmermann]

Hi

Am 08.09.26 um 15:23 schrieb Yongbang Shi:
>> Do not send the vblank event when the page flip completes, but when
>> the vblank interrupt fires. Sending the vblank event at the wrong time
>> can break synchronization of the display update with the vblank period.
>>
>> Use the vblank helper drm_crtc_vblank_atomic_flush(), which sets up
>> the vblank event accordingly. In case of an error, it still sends out
>> the vblank event immediately; thus not starving consumers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 16 ++--------------
>>   1 file changed, 2 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c 
>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> index b4ab53db1c08..72ce696e6d11 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -20,6 +20,7 @@
>>   #include <drm/drm_gem_atomic_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>>   #include <drm/drm_vblank.h>
>> +#include <drm/drm_vblank_helper.h>
>>     #include "hibmc_drm_drv.h"
>>   #include "hibmc_drm_regs.h"
>> @@ -420,19 +421,6 @@ static void hibmc_crtc_atomic_begin(struct 
>> drm_crtc *crtc,
>>       /* We can add more initialization as needed. */
>>   }
>>   -static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
>> -                    struct drm_atomic_commit *state)
>> -
>> -{
>> -    unsigned long flags;
>> -
>> -    spin_lock_irqsave(&crtc->dev->event_lock, flags);
>> -    if (crtc->state->event)
>> -        drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> -    crtc->state->event = NULL;
>> -    spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>> -}
>> -
>>   static int hibmc_crtc_enable_vblank(struct drm_crtc *crtc)
>>   {
>>       struct hibmc_drm_private *priv = to_hibmc_drm_private(crtc->dev);
>> @@ -502,7 +490,7 @@ static const struct drm_crtc_funcs 
>> hibmc_crtc_funcs = {
>>   static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>>       .mode_set_nofb    = hibmc_crtc_mode_set_nofb,
>>       .atomic_begin    = hibmc_crtc_atomic_begin,
>> -    .atomic_flush    = hibmc_crtc_atomic_flush,
>> +    .atomic_flush    = drm_crtc_vblank_atomic_flush,
>>       .atomic_enable    = hibmc_crtc_atomic_enable,
>>       .atomic_disable    = hibmc_crtc_atomic_disable,
>>       .mode_valid = hibmc_crtc_mode_valid,
>
> Reviewed-by: Yongbang Shi <shiyongbang@huawei.com>

Thanks for reviewing. I've merged the patches into drm-misc-next.

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, (HRB 36809, AG Nürnberg)