[PATCH 1/3] drm/komeda - Add legacy FB support so VT's work as expected

[PATCH 1/3] drm/komeda - Add legacy FB support so VT's work as expected

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

The komeda driver doesn't come up with a visible text (FB) mode VT by
default as it was missing legacy FB support. It's useful to have a
working text VT on a system for debug and general usability, so enable
it. You can always toggle CONFIG_FRAMEBUFFER_CONSOLE.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index e7933930a657..c0c7933a9631 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -9,6 +9,7 @@
 #include <linux/platform_device.h>
 #include <linux/component.h>
 #include <linux/pm_runtime.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_of.h>
 #include "komeda_dev.h"
 #include "komeda_kms.h"
@@ -71,6 +72,7 @@ static int komeda_bind(struct device *dev)
 	}
 
 	dev_set_drvdata(dev, mdrv);
+	drm_fbdev_generic_setup(&mdrv->kms->base, 32);
 
 	return 0;
 
-- 
2.32.0

[PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

If something has already set up the DPU before the komeda driver comes
up, it will fail to init because it was just writing to the SRST bit in
the GCU control register and ignoring others. This resulted in TBU
bringup stalling and init failing. By writing completely we also  set the
mode back to 0 (inactive) too and thus TBU bringup works.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index 00fa56c29b3e..39618c1a4c81 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
 	u32 __iomem *gcu = d71->gcu_addr;
 	int ret;
 
-	malidp_write32_mask(gcu, BLK_CONTROL,
-			    GCU_CONTROL_SRST, GCU_CONTROL_SRST);
+	malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
 
 	ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST),
 			   100, 1000, 10000);
-- 
2.32.0

Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[Liviu Dudau]

On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> If something has already set up the DPU before the komeda driver comes
> up, it will fail to init because it was just writing to the SRST bit in
> the GCU control register and ignoring others. This resulted in TBU
> bringup stalling and init failing. By writing completely we also  set the
> mode back to 0 (inactive) too and thus TBU bringup works.

This is a rather large hammer, tbh. I would like to see if there is a better way of
handling the handover from EFIFB that this patch is trying to fix, but I lack an
usable plaform for that. It will generate a flicker at module load time, but if users
of Morello are happy with that, then

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> ---
>  drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> index 00fa56c29b3e..39618c1a4c81 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> @@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
>  	u32 __iomem *gcu = d71->gcu_addr;
>  	int ret;
>  
> -	malidp_write32_mask(gcu, BLK_CONTROL,
> -			    GCU_CONTROL_SRST, GCU_CONTROL_SRST);
> +	malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
>  
>  	ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST),
>  			   100, 1000, 10000);
> -- 
> 2.32.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[Carsten Haitzler]

On 7/8/22 17:07, Liviu Dudau wrote:
> On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitzler@foss.arm.com wrote:
>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>
>> If something has already set up the DPU before the komeda driver comes
>> up, it will fail to init because it was just writing to the SRST bit in
>> the GCU control register and ignoring others. This resulted in TBU
>> bringup stalling and init failing. By writing completely we also  set the
>> mode back to 0 (inactive) too and thus TBU bringup works.
> 
> This is a rather large hammer, tbh. I would like to see if there is a better way of
> handling the handover from EFIFB that this patch is trying to fix, but I lack an
> usable plaform for that. It will generate a flicker at module load time, but if users
> of Morello are happy with that, then

We're pretty happy with that setup right now. Certainly better than 
Komeda failing to init.

> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
>>
>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>> ---
>>   drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
>> index 00fa56c29b3e..39618c1a4c81 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
>> @@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
>>   	u32 __iomem *gcu = d71->gcu_addr;
>>   	int ret;
>>   
>> -	malidp_write32_mask(gcu, BLK_CONTROL,
>> -			    GCU_CONTROL_SRST, GCU_CONTROL_SRST);
>> +	malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
>>   
>>   	ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST),
>>   			   100, 1000, 10000);
>> -- 
>> 2.32.0
>>
> 

Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU

[Carsten Haitzler]

On 7/8/22 17:07, Liviu Dudau wrote:
> On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitzler@foss.arm.com wrote:
>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>
>> If something has already set up the DPU before the komeda driver comes
>> up, it will fail to init because it was just writing to the SRST bit in
>> the GCU control register and ignoring others. This resulted in TBU
>> bringup stalling and init failing. By writing completely we also  set the
>> mode back to 0 (inactive) too and thus TBU bringup works.
> 
> This is a rather large hammer, tbh. I would like to see if there is a better way of
> handling the handover from EFIFB that this patch is trying to fix, but I lack an
> usable plaform for that. It will generate a flicker at module load time, but if users
> of Morello are happy with that, then

Just FYI - it'll flicker anyway as the PHY is external and gets 
re-initted etc. anyway... This also happens to handle the situation 
where something goes wrong and you have an already initted komeda sue to 
a previous module load (and it's still alive and working due to an 
unclean shutdown). It'll allow you to load the module again :) So it's 
multi-useful.

> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
>>
>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>> ---
>>   drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
>> index 00fa56c29b3e..39618c1a4c81 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
>> @@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71)
>>   	u32 __iomem *gcu = d71->gcu_addr;
>>   	int ret;
>>   
>> -	malidp_write32_mask(gcu, BLK_CONTROL,
>> -			    GCU_CONTROL_SRST, GCU_CONTROL_SRST);
>> +	malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST);
>>   
>>   	ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST),
>>   			   100, 1000, 10000);
>> -- 
>> 2.32.0
>>
> 

[PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).

This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 10 ++++++++++
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 19 ++++++++++++++++++-
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 +++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..b7f0a5f97222 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -227,6 +227,16 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 			complete_all(kcrtc->disable_done);
 			kcrtc->disable_done = NULL;
 		} else if (crtc->state->event) {
+			if (kcrtc->state_needs_handling) {
+				event = kcrtc->state_needs_handling->event;
+				if (event) {
+					kcrtc->state_needs_handling->event = NULL;
+					kcrtc->state_needs_handling = NULL;
+					drm_crtc_send_vblank_event(crtc, event);
+				} else {
+					kcrtc->state_needs_handling = NULL;
+				}
+			}
 			event = crtc->state->event;
 			/*
 			 * Consume event before notifying drm core that flip
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 93b7f09b96ca..bbc051a1896a 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -226,10 +226,27 @@ static int komeda_kms_check(struct drm_device *dev,
 	return 0;
 }
 
+static int komeda_kms_commit(struct drm_device *drm,
+                             struct drm_atomic_state *state,
+                             bool nonblock)
+{
+	int i;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct komeda_crtc *kcrtc;
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
+				      new_crtc_state, i) {
+		kcrtc = to_kcrtc(crtc);
+		kcrtc->state_needs_handling = crtc->state;
+	}
+	return drm_atomic_helper_commit(drm, state, nonblock);
+}
+
 static const struct drm_mode_config_funcs komeda_mode_config_funcs = {
 	.fb_create		= komeda_fb_create,
 	.atomic_check		= komeda_kms_check,
-	.atomic_commit		= drm_atomic_helper_commit,
+	.atomic_commit		= komeda_kms_commit,
 };
 
 static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719..8ff3ad04dfe4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -84,6 +84,9 @@ struct komeda_crtc {
 
 	/** @disable_done: this flip_done is for tracing the disable */
 	struct completion *disable_done;
+
+	/** @state_needs_handling: Has not had it's vblank event handled yet */
+	struct drm_crtc_state *state_needs_handling;
 };
 
 /**
-- 
2.32.0

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Liviu Dudau]

On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>

Hi Carsten,

> 
> Sometimes there is an extra dcm crtc state in the pipeline whose
> penting vblank event has not been handled yet. We would previously
> throw this out and the vblank event never triggers leading to hard
> lockups higher up the stack where an expected vlank event never comes
> back (screen freezes).
> 
> This fixes that by tracking a pending crtc state that needs handling
> and handle it producing a vlank event next vblank if it had not
> laready been handled before. This fixes the hangs and ensures our
> display keeps updating seamlessly and is certainly a much better state
> to be in than after some time ending up with a mysteriously frozen
> screen and a lot of kernle messages complaining about this too.

Sorry it took me so long to review and reply to this patch, but I had this nagging
feeling that the patch is not actually correct so I've tried to track the actual
issue. It turns out that the problem is easy to reproduce in a different setup with
Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
hardware to signal acceptance of config valid on setting new commits. I have created
a new patch that I would be happy if you can test to try to fix the actual issue.

--8<---------------------------------------------------------------------------

From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
From: Liviu Dudau <liviu.dudau@arm.com>
Date: Fri, 8 Jul 2022 16:39:21 +0100
Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
 atomic_commit_tail hook

Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements an atomic_commit_tail hook for the
mode_config_helper_funcs and even that one is pretty close to the generic
implementation with the exception of additional dma_fence signalling.

What the generic helper framework doesn't do is waiting for the actual
hardware to signal that the commit parameters have been written into the
appropriate registers. As we signal CRTC events only on the irq handlers,
we need to flush the configuration and wait for the hardware to respond.

Add the Komeda specific implementation for atomic_commit_hw_done() that
flushes and waits for flip done before calling drm_atomic_helper_commit_hw_done().

The fix was prompted by a patch from Carsten Haitzler where he was trying to
solve the same issue but in a different way that I think can lead to wrong
event signaling to userspace.

Reported-by: Carsten Haitzler <carsten.haitzler@arm.com>
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  4 ++--
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 20 ++++++++++++++++++-
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 ++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb973803d..292f533d8cf0de6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 			crtc->state->event = NULL;
 			drm_crtc_send_vblank_event(crtc, event);
 		} else {
-			DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
+			DRM_WARN("CRTC[%d]: FLIP happened but no pending commit.\n",
 				 drm_crtc_index(&kcrtc->base));
 		}
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
@@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
 	komeda_crtc_do_flush(crtc, old);
 }
 
-static void
+void
 komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
 					 struct completion *input_flip_done)
 {
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 93b7f09b96ca950..1a53bd87e81d8ad 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -69,6 +69,24 @@ static const struct drm_driver komeda_kms_driver = {
 	.minor = 1,
 };
 
+static void komeda_kms_atomic_commit_hw_done(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct komeda_kms_dev *kms = to_kdev(dev);
+
+	for (int i = 0; i < kms->n_crtcs; i++) {
+		struct komeda_crtc *kcrtc = &kms->crtcs[i];
+
+		if (kcrtc->base.state->active) {
+			struct completion *flip_done = NULL;
+			if (kcrtc->base.state->event)
+				flip_done = kcrtc->base.state->event->base.completion;
+			komeda_crtc_flush_and_wait_for_flip_done(kcrtc, flip_done);
+		}
+	}
+	drm_atomic_helper_commit_hw_done(state);
+}
+
 static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
@@ -81,7 +99,7 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
 
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
-	drm_atomic_helper_commit_hw_done(old_state);
+	komeda_kms_atomic_commit_hw_done(old_state);
 
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719317..bf6e8fba5061335 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -182,6 +182,8 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms);
 
 void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 			      struct komeda_events *evts);
+void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
+					      struct completion *input_flip_done);
 
 struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
 void komeda_kms_detach(struct komeda_kms_dev *kms);
-- 
2.37.0

-->8---------------------------------------------------------------------------

Best regards,
Liviu


> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> ---
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 10 ++++++++++
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 19 ++++++++++++++++++-
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  3 +++
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 59172acb9738..b7f0a5f97222 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -227,6 +227,16 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  			complete_all(kcrtc->disable_done);
>  			kcrtc->disable_done = NULL;
>  		} else if (crtc->state->event) {
> +			if (kcrtc->state_needs_handling) {
> +				event = kcrtc->state_needs_handling->event;
> +				if (event) {
> +					kcrtc->state_needs_handling->event = NULL;
> +					kcrtc->state_needs_handling = NULL;
> +					drm_crtc_send_vblank_event(crtc, event);
> +				} else {
> +					kcrtc->state_needs_handling = NULL;
> +				}
> +			}
>  			event = crtc->state->event;
>  			/*
>  			 * Consume event before notifying drm core that flip
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 93b7f09b96ca..bbc051a1896a 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -226,10 +226,27 @@ static int komeda_kms_check(struct drm_device *dev,
>  	return 0;
>  }
>  
> +static int komeda_kms_commit(struct drm_device *drm,
> +                             struct drm_atomic_state *state,
> +                             bool nonblock)
> +{
> +	int i;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct komeda_crtc *kcrtc;
> +
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> +				      new_crtc_state, i) {
> +		kcrtc = to_kcrtc(crtc);
> +		kcrtc->state_needs_handling = crtc->state;
> +	}
> +	return drm_atomic_helper_commit(drm, state, nonblock);
> +}
> +
>  static const struct drm_mode_config_funcs komeda_mode_config_funcs = {
>  	.fb_create		= komeda_fb_create,
>  	.atomic_check		= komeda_kms_check,
> -	.atomic_commit		= drm_atomic_helper_commit,
> +	.atomic_commit		= komeda_kms_commit,
>  };
>  
>  static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index 456f3c435719..8ff3ad04dfe4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -84,6 +84,9 @@ struct komeda_crtc {
>  
>  	/** @disable_done: this flip_done is for tracing the disable */
>  	struct completion *disable_done;
> +
> +	/** @state_needs_handling: Has not had it's vblank event handled yet */
> +	struct drm_crtc_state *state_needs_handling;
>  };
>  
>  /**
> -- 
> 2.32.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Carsten Haitzler]

On 7/8/22 17:02, Liviu Dudau wrote:
> On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitzler@foss.arm.com wrote:
>> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Hi Carsten,
> 
>>
>> Sometimes there is an extra dcm crtc state in the pipeline whose
>> penting vblank event has not been handled yet. We would previously
>> throw this out and the vblank event never triggers leading to hard
>> lockups higher up the stack where an expected vlank event never comes
>> back (screen freezes).
>>
>> This fixes that by tracking a pending crtc state that needs handling
>> and handle it producing a vlank event next vblank if it had not
>> laready been handled before. This fixes the hangs and ensures our
>> display keeps updating seamlessly and is certainly a much better state
>> to be in than after some time ending up with a mysteriously frozen
>> screen and a lot of kernle messages complaining about this too.
> 
> Sorry it took me so long to review and reply to this patch, but I had this nagging

No worries. :)

> feeling that the patch is not actually correct so I've tried to track the actual
> issue. It turns out that the problem is easy to reproduce in a different setup with
> Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
> hardware to signal acceptance of config valid on setting new commits. I have created
> a new patch that I would be happy if you can test to try to fix the actual issue.

This works (tested). One errant "flip without commit":

[    9.402559] fbcon: Taking over console
[    9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: 
gcu: MERR, pipes[0]: FLIP, pipes[1]: None
[    9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
[    9.542215] Console: switching to colour frame buffer device 240x67

But nothing worrying. It does work, though doesn't compile due to:

drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function 
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop 
initial declarations are only allowed in C99 or C11 mode
    77 |         for (int i = 0; i < kms->n_crtcs; i++) {
       |         ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option 
‘-std=c9
’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code

but that was a trivial fixup.

Your commit handler does sit and wait. I guess I avoided that and had it 
still deferred and handled next time the vblank interrupt fires. Yours 
is a bit shorter and less complex so it wins. :)

> --8<---------------------------------------------------------------------------
> 
>  From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
> From: Liviu Dudau <liviu.dudau@arm.com>
> Date: Fri, 8 Jul 2022 16:39:21 +0100
> Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
>   atomic_commit_tail hook
> 
> Komeda driver relies on the generic DRM atomic helper functions to handle
> commits. It only implements an atomic_commit_tail hook for the
> mode_config_helper_funcs and even that one is pretty close to the generic
> implementation with the exception of additional dma_fence signalling.
> 
> What the generic helper framework doesn't do is waiting for the actual
> hardware to signal that the commit parameters have been written into the
> appropriate registers. As we signal CRTC events only on the irq handlers,
> we need to flush the configuration and wait for the hardware to respond.
> 
> Add the Komeda specific implementation for atomic_commit_hw_done() that
> flushes and waits for flip done before calling drm_atomic_helper_commit_hw_done().
> 
> The fix was prompted by a patch from Carsten Haitzler where he was trying to
> solve the same issue but in a different way that I think can lead to wrong
> event signaling to userspace.
> 
> Reported-by: Carsten Haitzler <carsten.haitzler@arm.com>
> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
>   .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  4 ++--
>   .../gpu/drm/arm/display/komeda/komeda_kms.c   | 20 ++++++++++++++++++-
>   .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 ++
>   3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 59172acb973803d..292f533d8cf0de6 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>   			crtc->state->event = NULL;
>   			drm_crtc_send_vblank_event(crtc, event);
>   		} else {
> -			DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> +			DRM_WARN("CRTC[%d]: FLIP happened but no pending commit.\n",
>   				 drm_crtc_index(&kcrtc->base));
>   		}
>   		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> @@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
>   	komeda_crtc_do_flush(crtc, old);
>   }
>   
> -static void
> +void
>   komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
>   					 struct completion *input_flip_done)
>   {
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 93b7f09b96ca950..1a53bd87e81d8ad 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -69,6 +69,24 @@ static const struct drm_driver komeda_kms_driver = {
>   	.minor = 1,
>   };
>   
> +static void komeda_kms_atomic_commit_hw_done(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct komeda_kms_dev *kms = to_kdev(dev);
> +
> +	for (int i = 0; i < kms->n_crtcs; i++) {
> +		struct komeda_crtc *kcrtc = &kms->crtcs[i];
> +
> +		if (kcrtc->base.state->active) {
> +			struct completion *flip_done = NULL;
> +			if (kcrtc->base.state->event)
> +				flip_done = kcrtc->base.state->event->base.completion;
> +			komeda_crtc_flush_and_wait_for_flip_done(kcrtc, flip_done);
> +		}
> +	}
> +	drm_atomic_helper_commit_hw_done(state);
> +}
> +
>   static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
>   {
>   	struct drm_device *dev = old_state->dev;
> @@ -81,7 +99,7 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
>   
>   	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>   
> -	drm_atomic_helper_commit_hw_done(old_state);
> +	komeda_kms_atomic_commit_hw_done(old_state);
>   
>   	drm_atomic_helper_wait_for_flip_done(dev, old_state);
>   
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index 456f3c435719317..bf6e8fba5061335 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -182,6 +182,8 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms);
>   
>   void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>   			      struct komeda_events *evts);
> +void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
> +					      struct completion *input_flip_done);
>   
>   struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
>   void komeda_kms_detach(struct komeda_kms_dev *kms);

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Liviu Dudau]

On Fri, Jul 08, 2022 at 07:03:37PM +0100, Carsten Haitzler wrote:
> 
> 
> On 7/8/22 17:02, Liviu Dudau wrote:
> > On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitzler@foss.arm.com wrote:
> > > From: Carsten Haitzler <carsten.haitzler@arm.com>
> > 
> > Hi Carsten,
> > 
> > > 
> > > Sometimes there is an extra dcm crtc state in the pipeline whose
> > > penting vblank event has not been handled yet. We would previously
> > > throw this out and the vblank event never triggers leading to hard
> > > lockups higher up the stack where an expected vlank event never comes
> > > back (screen freezes).
> > > 
> > > This fixes that by tracking a pending crtc state that needs handling
> > > and handle it producing a vlank event next vblank if it had not
> > > laready been handled before. This fixes the hangs and ensures our
> > > display keeps updating seamlessly and is certainly a much better state
> > > to be in than after some time ending up with a mysteriously frozen
> > > screen and a lot of kernle messages complaining about this too.
> > 
> > Sorry it took me so long to review and reply to this patch, but I had this nagging
> 
> No worries. :)
> 
> > feeling that the patch is not actually correct so I've tried to track the actual
> > issue. It turns out that the problem is easy to reproduce in a different setup with
> > Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
> > hardware to signal acceptance of config valid on setting new commits. I have created
> > a new patch that I would be happy if you can test to try to fix the actual issue.
> 
> This works (tested).

Thank you very much for testing this! Can I add your Tested-by?

>  One errant "flip without commit":
> 
> [    9.402559] fbcon: Taking over console
> [    9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: gcu:
> MERR, pipes[0]: FLIP, pipes[1]: None
> [    9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
> [    9.542215] Console: switching to colour frame buffer device 240x67
> 

Is this with your 2/3 patch applied and coming out from the firmware having already
initialised the hardware? If so, the handover probably doesn't quiescence the
interrupts correctly so an interrupt is pending when the kernel driver is
initialised. That's another area worth looking at for the handover purposes.


> But nothing worrying. It does work, though doesn't compile due to:
> 
> drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
> ‘komeda_kms_atomic_commit_hw_done’:
> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
> initial declarations are only allowed in C99 or C11 mode
>    77 |         for (int i = 0; i < kms->n_crtcs; i++) {
>       |         ^~~
> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
> ‘-std=c9
> ’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
> 
> but that was a trivial fixup.

Interesting that I'm not seeing that, probably due to using GCC12? Anyway, I'll fix
that and send a proper patch.

> 
> Your commit handler does sit and wait. I guess I avoided that and had it
> still deferred and handled next time the vblank interrupt fires. Yours is a
> bit shorter and less complex so it wins. :)

Yes, that's the essence of my issue with your patch. Delaying the handling of the
event until the next vblank means older software that doesn't use the timestamping
from the vblank event will get wrong framerates (playing video might be affected).

Waiting here when we're also calling drm_atomic_helper_wait_for_flip_done() after
drm_atomic_helper_commit_hw_done() feels wrong, but then the later is checking if we
have consumed the event so we have to. Maybe the introduction of the
drm_atomic_helper_fake_vblank() is needed in komeda as well like the generic
commit_tail helper function does? I need to investigate more the next time I get some
spare cycles on komeda.

I will send a new email with the updated patch and your Tested-by if that's OK.

Best regards,
Liviu

> 
> > --8<---------------------------------------------------------------------------
> > 
> >  From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
> > From: Liviu Dudau <liviu.dudau@arm.com>
> > Date: Fri, 8 Jul 2022 16:39:21 +0100
> > Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
> >   atomic_commit_tail hook
> > 
> > Komeda driver relies on the generic DRM atomic helper functions to handle
> > commits. It only implements an atomic_commit_tail hook for the
> > mode_config_helper_funcs and even that one is pretty close to the generic
> > implementation with the exception of additional dma_fence signalling.
> > 
> > What the generic helper framework doesn't do is waiting for the actual
> > hardware to signal that the commit parameters have been written into the
> > appropriate registers. As we signal CRTC events only on the irq handlers,
> > we need to flush the configuration and wait for the hardware to respond.
> > 
> > Add the Komeda specific implementation for atomic_commit_hw_done() that
> > flushes and waits for flip done before calling drm_atomic_helper_commit_hw_done().
> > 
> > The fix was prompted by a patch from Carsten Haitzler where he was trying to
> > solve the same issue but in a different way that I think can lead to wrong
> > event signaling to userspace.
> > 
> > Reported-by: Carsten Haitzler <carsten.haitzler@arm.com>
> > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> > ---
> >   .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  4 ++--
> >   .../gpu/drm/arm/display/komeda/komeda_kms.c   | 20 ++++++++++++++++++-
> >   .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 ++
> >   3 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > index 59172acb973803d..292f533d8cf0de6 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > @@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
> >   			crtc->state->event = NULL;
> >   			drm_crtc_send_vblank_event(crtc, event);
> >   		} else {
> > -			DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> > +			DRM_WARN("CRTC[%d]: FLIP happened but no pending commit.\n",
> >   				 drm_crtc_index(&kcrtc->base));
> >   		}
> >   		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > @@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
> >   	komeda_crtc_do_flush(crtc, old);
> >   }
> > -static void
> > +void
> >   komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
> >   					 struct completion *input_flip_done)
> >   {
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > index 93b7f09b96ca950..1a53bd87e81d8ad 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > @@ -69,6 +69,24 @@ static const struct drm_driver komeda_kms_driver = {
> >   	.minor = 1,
> >   };
> > +static void komeda_kms_atomic_commit_hw_done(struct drm_atomic_state *state)
> > +{
> > +	struct drm_device *dev = state->dev;
> > +	struct komeda_kms_dev *kms = to_kdev(dev);
> > +
> > +	for (int i = 0; i < kms->n_crtcs; i++) {
> > +		struct komeda_crtc *kcrtc = &kms->crtcs[i];
> > +
> > +		if (kcrtc->base.state->active) {
> > +			struct completion *flip_done = NULL;
> > +			if (kcrtc->base.state->event)
> > +				flip_done = kcrtc->base.state->event->base.completion;
> > +			komeda_crtc_flush_and_wait_for_flip_done(kcrtc, flip_done);
> > +		}
> > +	}
> > +	drm_atomic_helper_commit_hw_done(state);
> > +}
> > +
> >   static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
> >   {
> >   	struct drm_device *dev = old_state->dev;
> > @@ -81,7 +99,7 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
> >   	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > -	drm_atomic_helper_commit_hw_done(old_state);
> > +	komeda_kms_atomic_commit_hw_done(old_state);
> >   	drm_atomic_helper_wait_for_flip_done(dev, old_state);
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > index 456f3c435719317..bf6e8fba5061335 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > @@ -182,6 +182,8 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms);
> >   void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
> >   			      struct komeda_events *evts);
> > +void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
> > +					      struct completion *input_flip_done);
> >   struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
> >   void komeda_kms_detach(struct komeda_kms_dev *kms);

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Robin Murphy]

On 2022-07-11 11:13, Liviu Dudau wrote:
[...]
>> But nothing worrying. It does work, though doesn't compile due to:
>>
>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
>> ‘komeda_kms_atomic_commit_hw_done’:
>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
>> initial declarations are only allowed in C99 or C11 mode
>>     77 |         for (int i = 0; i < kms->n_crtcs; i++) {
>>        |         ^~~
>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
>> ‘-std=c9
>> ’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
>>
>> but that was a trivial fixup.
> 
> Interesting that I'm not seeing that, probably due to using GCC12? Anyway, I'll fix
> that and send a proper patch.

FWIW we do use -std=gnu11 since 5.18 (see e8c07082a810), but I'm not 
entirely sure what the status quo is for using the new features in fixes 
which might also warrant backporting to stable. I believe Carsten's 
stuck on an older kernel thanks to constraints of the rest of that 
project ;)

Cheers,
Robin.

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Carsten Haitzler]

On 7/14/22 13:20, Robin Murphy wrote:
> On 2022-07-11 11:13, Liviu Dudau wrote:
> [...]
>>> But nothing worrying. It does work, though doesn't compile due to:
>>>
>>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
>>> ‘komeda_kms_atomic_commit_hw_done’:
>>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
>>> initial declarations are only allowed in C99 or C11 mode
>>>     77 |         for (int i = 0; i < kms->n_crtcs; i++) {
>>>        |         ^~~
>>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
>>> ‘-std=c9
>>> ’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
>>>
>>> but that was a trivial fixup.
>>
>> Interesting that I'm not seeing that, probably due to using GCC12? 
>> Anyway, I'll fix
>> that and send a proper patch.
> 
> FWIW we do use -std=gnu11 since 5.18 (see e8c07082a810), but I'm not 
> entirely sure what the status quo is for using the new features in fixes 
> which might also warrant backporting to stable. I believe Carsten's 
> stuck on an older kernel thanks to constraints of the rest of that 
> project ;)

Not that old - my last sync was like end of April, but i was basing my 
commits off a stable kernel release tree (5.17.4), I have multiple 
kernels for different purposes and for this stuck to something released 
vaguely recently (i synced my tree to latest release before sending off 
the patch set). I'm not sure on the kernel policy for the above for (int 
i = 0;...) etc. usage. I tend to still be more conservative and keep my 
vars at top of the block anyway out of decades of habit.

Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up

[Carsten Haitzler]

On 7/11/22 11:13, Liviu Dudau wrote:
> On Fri, Jul 08, 2022 at 07:03:37PM +0100, Carsten Haitzler wrote:
>>
>>
>> On 7/8/22 17:02, Liviu Dudau wrote:
>>> On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitzler@foss.arm.com wrote:
>>>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>>
>>> Hi Carsten,
>>>
>>>>
>>>> Sometimes there is an extra dcm crtc state in the pipeline whose
>>>> penting vblank event has not been handled yet. We would previously
>>>> throw this out and the vblank event never triggers leading to hard
>>>> lockups higher up the stack where an expected vlank event never comes
>>>> back (screen freezes).
>>>>
>>>> This fixes that by tracking a pending crtc state that needs handling
>>>> and handle it producing a vlank event next vblank if it had not
>>>> laready been handled before. This fixes the hangs and ensures our
>>>> display keeps updating seamlessly and is certainly a much better state
>>>> to be in than after some time ending up with a mysteriously frozen
>>>> screen and a lot of kernle messages complaining about this too.
>>>
>>> Sorry it took me so long to review and reply to this patch, but I had this nagging
>>
>> No worries. :)
>>
>>> feeling that the patch is not actually correct so I've tried to track the actual
>>> issue. It turns out that the problem is easy to reproduce in a different setup with
>>> Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
>>> hardware to signal acceptance of config valid on setting new commits. I have created
>>> a new patch that I would be happy if you can test to try to fix the actual issue.
>>
>> This works (tested).
> 
> Thank you very much for testing this! Can I add your Tested-by?

Indeed. Go for it.

>>   One errant "flip without commit":
>>
>> [    9.402559] fbcon: Taking over console
>> [    9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: gcu:
>> MERR, pipes[0]: FLIP, pipes[1]: None
>> [    9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
>> [    9.542215] Console: switching to colour frame buffer device 240x67
>>
> 
> Is this with your 2/3 patch applied and coming out from the firmware having already
> initialised the hardware? If so, the handover probably doesn't quiescence the
> interrupts correctly so an interrupt is pending when the kernel driver is
> initialised. That's another area worth looking at for the handover purposes.

Yeah. the firmware is not doing a very clean handover - it's leaving the 
hardware in whatever state it has. it probably could shut down 
interrupts on handover, but not something to worry about in the kernel 
driver at this point.

>> But nothing worrying. It does work, though doesn't compile due to:
>>
>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
>> ‘komeda_kms_atomic_commit_hw_done’:
>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
>> initial declarations are only allowed in C99 or C11 mode
>>     77 |         for (int i = 0; i < kms->n_crtcs; i++) {
>>        |         ^~~
>> drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
>> ‘-std=c9
>> ’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
>>
>> but that was a trivial fixup.
> 
> Interesting that I'm not seeing that, probably due to using GCC12? Anyway, I'll fix
> that and send a proper patch.

I tend to use newer gcc's indeed, but I am using gcc11 in this case.

>> Your commit handler does sit and wait. I guess I avoided that and had it
>> still deferred and handled next time the vblank interrupt fires. Yours is a
>> bit shorter and less complex so it wins. :)
> 
> Yes, that's the essence of my issue with your patch. Delaying the handling of the
> event until the next vblank means older software that doesn't use the timestamping
> from the vblank event will get wrong framerates (playing video might be affected).

But this is a rare situation. I certainly was very happy going from "my 
entire display locks up and only a reboot really fixes it" to "look ma - 
it works and I didn't even see a stuttered frame!" :) But your fix is 
better in that regard.

> Waiting here when we're also calling drm_atomic_helper_wait_for_flip_done() after
> drm_atomic_helper_commit_hw_done() feels wrong, but then the later is checking if we
> have consumed the event so we have to. Maybe the introduction of the
> drm_atomic_helper_fake_vblank() is needed in komeda as well like the generic
> commit_tail helper function does? I need to investigate more the next time I get some
> spare cycles on komeda.
> 
> I will send a new email with the updated patch and your Tested-by if that's OK.

All happy and fine with that.

> Best regards,
> Liviu
> 
>>
>>> --8<---------------------------------------------------------------------------
>>>
>>>   From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
>>> From: Liviu Dudau <liviu.dudau@arm.com>
>>> Date: Fri, 8 Jul 2022 16:39:21 +0100
>>> Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
>>>    atomic_commit_tail hook
>>>
>>> Komeda driver relies on the generic DRM atomic helper functions to handle
>>> commits. It only implements an atomic_commit_tail hook for the
>>> mode_config_helper_funcs and even that one is pretty close to the generic
>>> implementation with the exception of additional dma_fence signalling.
>>>
>>> What the generic helper framework doesn't do is waiting for the actual
>>> hardware to signal that the commit parameters have been written into the
>>> appropriate registers. As we signal CRTC events only on the irq handlers,
>>> we need to flush the configuration and wait for the hardware to respond.
>>>
>>> Add the Komeda specific implementation for atomic_commit_hw_done() that
>>> flushes and waits for flip done before calling drm_atomic_helper_commit_hw_done().
>>>
>>> The fix was prompted by a patch from Carsten Haitzler where he was trying to
>>> solve the same issue but in a different way that I think can lead to wrong
>>> event signaling to userspace.
>>>
>>> Reported-by: Carsten Haitzler <carsten.haitzler@arm.com>
>>> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
>>> ---
>>>    .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  4 ++--
>>>    .../gpu/drm/arm/display/komeda/komeda_kms.c   | 20 ++++++++++++++++++-
>>>    .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 ++
>>>    3 files changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
>>> index 59172acb973803d..292f533d8cf0de6 100644
>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
>>> @@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>>>    			crtc->state->event = NULL;
>>>    			drm_crtc_send_vblank_event(crtc, event);
>>>    		} else {
>>> -			DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
>>> +			DRM_WARN("CRTC[%d]: FLIP happened but no pending commit.\n",
>>>    				 drm_crtc_index(&kcrtc->base));
>>>    		}
>>>    		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>> @@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
>>>    	komeda_crtc_do_flush(crtc, old);
>>>    }
>>> -static void
>>> +void
>>>    komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
>>>    					 struct completion *input_flip_done)
>>>    {
>>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
>>> index 93b7f09b96ca950..1a53bd87e81d8ad 100644
>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
>>> @@ -69,6 +69,24 @@ static const struct drm_driver komeda_kms_driver = {
>>>    	.minor = 1,
>>>    };
>>> +static void komeda_kms_atomic_commit_hw_done(struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_device *dev = state->dev;
>>> +	struct komeda_kms_dev *kms = to_kdev(dev);
>>> +
>>> +	for (int i = 0; i < kms->n_crtcs; i++) {
>>> +		struct komeda_crtc *kcrtc = &kms->crtcs[i];
>>> +
>>> +		if (kcrtc->base.state->active) {
>>> +			struct completion *flip_done = NULL;
>>> +			if (kcrtc->base.state->event)
>>> +				flip_done = kcrtc->base.state->event->base.completion;
>>> +			komeda_crtc_flush_and_wait_for_flip_done(kcrtc, flip_done);
>>> +		}
>>> +	}
>>> +	drm_atomic_helper_commit_hw_done(state);
>>> +}
>>> +
>>>    static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
>>>    {
>>>    	struct drm_device *dev = old_state->dev;
>>> @@ -81,7 +99,7 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
>>>    	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>> -	drm_atomic_helper_commit_hw_done(old_state);
>>> +	komeda_kms_atomic_commit_hw_done(old_state);
>>>    	drm_atomic_helper_wait_for_flip_done(dev, old_state);
>>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
>>> index 456f3c435719317..bf6e8fba5061335 100644
>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
>>> @@ -182,6 +182,8 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms);
>>>    void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>>>    			      struct komeda_events *evts);
>>> +void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
>>> +					      struct completion *input_flip_done);
>>>    struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
>>>    void komeda_kms_detach(struct komeda_kms_dev *kms);
> 

Re: [PATCH 1/3] drm/komeda - Add legacy FB support so VT's work as expected

[Liviu Dudau]

On Mon, Jun 06, 2022 at 12:47:12PM +0100, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> The komeda driver doesn't come up with a visible text (FB) mode VT by
> default as it was missing legacy FB support. It's useful to have a
> working text VT on a system for debug and general usability, so enable
> it. You can always toggle CONFIG_FRAMEBUFFER_CONSOLE.
> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>


> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index e7933930a657..c0c7933a9631 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -9,6 +9,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/component.h>
>  #include <linux/pm_runtime.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_of.h>
>  #include "komeda_dev.h"
>  #include "komeda_kms.h"
> @@ -71,6 +72,7 @@ static int komeda_bind(struct device *dev)
>  	}
>  
>  	dev_set_drvdata(dev, mdrv);
> +	drm_fbdev_generic_setup(&mdrv->kms->base, 32);
>  
>  	return 0;
>  
> -- 
> 2.32.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯