[PATCH RESEND v4 v5 0/4] drm/vc4: Use the firmware to stop the display pipeline

[PATCH RESEND v4 v5 0/4] drm/vc4: Use the firmware to stop the display pipeline

[Maxime Ripard]

Hi,

The VC4 driver has had limited support to disable the HDMI controllers and
pixelvalves at boot if the firmware has enabled them.

However, this proved to be limited, and a bit unreliable so a new firmware
command has been introduced some time ago to make it free all its resources and
disable any display output it might have enabled.

This series takes advantage of that command to call it once the transition from
simplefb to the KMS driver has been done.

Let me know what you think,
Maxime

---

Changes from v4:
  - Don't register v3d with nomodeset

Changes from v3:
  - Support nomodeset

Changes from v2:
  - Switch back to rpi_firmware_get / rpi_firmware_put
  - Moved the rpi_firmware pointer to a local variable

Changes from v1:
  - Use of_find_compatible_node instead of a phandle
  - Use devm_rpi_firmware_get

Maxime Ripard (4):
  firmware: raspberrypi: Add RPI_FIRMWARE_NOTIFY_DISPLAY_DONE
  drm/vc4: Support nomodeset
  drm/vc4: Remove conflicting framebuffers before callind bind_all
  drm/vc4: Notify the firmware when DRM is in charge

 drivers/gpu/drm/vc4/vc4_drv.c              | 33 +++++++++++++++++++---
 include/soc/bcm2835/raspberrypi-firmware.h |  1 +
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.33.1

[PATCH RESEND v4 v5 1/4] firmware: raspberrypi: Add RPI_FIRMWARE_NOTIFY_DISPLAY_DONE

[Maxime Ripard]

The RPI_FIRMWARE_NOTIFY_DISPLAY_DONE firmware call allows to tell the
firmware the kernel is in charge of the display now and the firmware can
free whatever resources it was using.

Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 include/soc/bcm2835/raspberrypi-firmware.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index 73ad784fca96..811ea668c4a1 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -91,6 +91,7 @@ enum rpi_firmware_property_tag {
 	RPI_FIRMWARE_GET_POE_HAT_VAL =                        0x00030049,
 	RPI_FIRMWARE_SET_POE_HAT_VAL =                        0x00030050,
 	RPI_FIRMWARE_NOTIFY_XHCI_RESET =                      0x00030058,
+	RPI_FIRMWARE_NOTIFY_DISPLAY_DONE =                    0x00030066,
 
 	/* Dispmanx TAGS */
 	RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE =                   0x00040001,
-- 
2.33.1

[PATCH RESEND v4 v5 2/4] drm/vc4: Support nomodeset

[Maxime Ripard]

If we have nomodeset on the kernel command line we should have the
firmware framebuffer driver kept as is and not try to load the
full-blown KMS driver.

In this case, let's just register the v3d driver.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 16abc3a3d601..d3cae84a4c4e 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -357,6 +357,9 @@ static int __init vc4_drm_register(void)
 {
 	int ret;
 
+	if (drm_firmware_drivers_only())
+		return -ENODEV;
+
 	ret = platform_register_drivers(component_drivers,
 					ARRAY_SIZE(component_drivers));
 	if (ret)
-- 
2.33.1

[PATCH RESEND v4 v5 3/4] drm/vc4: Remove conflicting framebuffers before callind bind_all

[Maxime Ripard]

The bind hooks will modify their controller registers, so simplefb is
going to be unusable anyway. Let's avoid any transient state where it
could still be in the system but no longer functionnal.

Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d3cae84a4c4e..86c61ee120b7 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -251,6 +251,10 @@ static int vc4_drm_bind(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = drm_aperture_remove_framebuffers(false, &vc4_drm_driver);
+	if (ret)
+		return ret;
+
 	ret = component_bind_all(dev, drm);
 	if (ret)
 		return ret;
@@ -259,10 +263,6 @@ static int vc4_drm_bind(struct device *dev)
 	if (ret)
 		goto unbind_all;
 
-	ret = drm_aperture_remove_framebuffers(false, &vc4_drm_driver);
-	if (ret)
-		goto unbind_all;
-
 	ret = vc4_kms_load(drm);
 	if (ret < 0)
 		goto unbind_all;
-- 
2.33.1

[PATCH RESEND v4 v5 4/4] drm/vc4: Notify the firmware when DRM is in charge

[Maxime Ripard]

Once the call to drm_fb_helper_remove_conflicting_framebuffers() has
been made, simplefb has been unregistered and the KMS driver is entirely
in charge of the display.

Thus, we can notify the firmware it can free whatever resource it was
using to maintain simplefb functional.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 86c61ee120b7..a03053c8e22c 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -37,6 +37,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_vblank.h>
 
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
 #include "uapi/drm/vc4_drm.h"
 
 #include "vc4_drv.h"
@@ -215,6 +217,7 @@ static void vc4_match_add_drivers(struct device *dev,
 static int vc4_drm_bind(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct rpi_firmware *firmware = NULL;
 	struct drm_device *drm;
 	struct vc4_dev *vc4;
 	struct device_node *node;
@@ -251,10 +254,29 @@ static int vc4_drm_bind(struct device *dev)
 	if (ret)
 		return ret;
 
+	node = of_find_compatible_node(NULL, NULL, "raspberrypi,bcm2835-firmware");
+	if (node) {
+		firmware = rpi_firmware_get(node);
+		of_node_put(node);
+
+		if (!firmware)
+			return -EPROBE_DEFER;
+	}
+
 	ret = drm_aperture_remove_framebuffers(false, &vc4_drm_driver);
 	if (ret)
 		return ret;
 
+	if (firmware) {
+		ret = rpi_firmware_property(firmware,
+					    RPI_FIRMWARE_NOTIFY_DISPLAY_DONE,
+					    NULL, 0);
+		if (ret)
+			drm_warn(drm, "Couldn't stop firmware display driver: %d\n", ret);
+
+		rpi_firmware_put(firmware);
+	}
+
 	ret = component_bind_all(dev, drm);
 	if (ret)
 		return ret;
-- 
2.33.1

Re: [PATCH RESEND v4 v5 0/4] drm/vc4: Use the firmware to stop the display pipeline

[Maxime Ripard]

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

On Wed, Dec 15, 2021 at 10:51:13AM +0100, Maxime Ripard wrote:
> Hi,
> 
> The VC4 driver has had limited support to disable the HDMI controllers and
> pixelvalves at boot if the firmware has enabled them.
> 
> However, this proved to be limited, and a bit unreliable so a new firmware
> command has been introduced some time ago to make it free all its resources and
> disable any display output it might have enabled.
> 
> This series takes advantage of that command to call it once the transition from
> simplefb to the KMS driver has been done.
> 
> Let me know what you think,
> Maxime

And I forgot to drop the RESEND v4 prefix.. This should obviously read PATCH v5

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RESEND v4 v5 2/4] drm/vc4: Support nomodeset

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1469 bytes --]

Hi

Am 15.12.21 um 10:51 schrieb Maxime Ripard:
> If we have nomodeset on the kernel command line we should have the
> firmware framebuffer driver kept as is and not try to load the
> full-blown KMS driver.
> 
> In this case, let's just register the v3d driver.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 16abc3a3d601..d3cae84a4c4e 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -357,6 +357,9 @@ static int __init vc4_drm_register(void)
>   {
>   	int ret;
>   
> +	if (drm_firmware_drivers_only())
> +		return -ENODEV;
> +

Just now that you've updated it, Lucas Stach had a similar concern about 
etnaviv, which doesn't do modesetting either. So we probably want to 
discuss what to do about drivers that have no modesetting separately.

If you want to land the patch, for either version of the code, with or 
without vc3:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regard
Thomas

>   	ret = platform_register_drivers(component_drivers,
>   					ARRAY_SIZE(component_drivers));
>   	if (ret)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH RESEND v4 v5 4/4] drm/vc4: Notify the firmware when DRM is in charge

[Javier Martinez Canillas]

Hello Maxime,

On Wed, Dec 15, 2021 at 10:51 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Once the call to drm_fb_helper_remove_conflicting_framebuffers() has
> been made, simplefb has been unregistered and the KMS driver is entirely
> in charge of the display.
>
> Thus, we can notify the firmware it can free whatever resource it was
> using to maintain simplefb functional.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH RESEND v4 v5 4/4] drm/vc4: Notify the firmware when DRM is in charge

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2555 bytes --]

Hi

Am 15.12.21 um 10:51 schrieb Maxime Ripard:
> Once the call to drm_fb_helper_remove_conflicting_framebuffers() has
> been made, simplefb has been unregistered and the KMS driver is entirely
> in charge of the display.
> 
> Thus, we can notify the firmware it can free whatever resource it was
> using to maintain simplefb functional.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 86c61ee120b7..a03053c8e22c 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -37,6 +37,8 @@
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_vblank.h>
>   
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
>   #include "uapi/drm/vc4_drm.h"
>   
>   #include "vc4_drv.h"
> @@ -215,6 +217,7 @@ static void vc4_match_add_drivers(struct device *dev,
>   static int vc4_drm_bind(struct device *dev)
>   {
>   	struct platform_device *pdev = to_platform_device(dev);
> +	struct rpi_firmware *firmware = NULL;
>   	struct drm_device *drm;
>   	struct vc4_dev *vc4;
>   	struct device_node *node;
> @@ -251,10 +254,29 @@ static int vc4_drm_bind(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> +	node = of_find_compatible_node(NULL, NULL, "raspberrypi,bcm2835-firmware");
> +	if (node) {
> +		firmware = rpi_firmware_get(node);
> +		of_node_put(node);
> +
> +		if (!firmware)
> +			return -EPROBE_DEFER;
> +	}
> +

The code is

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Just for my understanding:

You retrieve the firmware before killing simpledrm simply to keep the 
display on if it fails, right?

What's the possible error that would justify a retry (via EPROBE_DEFER)?

Best regards
Thomas

>   	ret = drm_aperture_remove_framebuffers(false, &vc4_drm_driver);
>   	if (ret)
>   		return ret;
>   
> +	if (firmware) {
> +		ret = rpi_firmware_property(firmware,
> +					    RPI_FIRMWARE_NOTIFY_DISPLAY_DONE,
> +					    NULL, 0);
> +		if (ret)
> +			drm_warn(drm, "Couldn't stop firmware display driver: %d\n", ret);
> +
> +		rpi_firmware_put(firmware);
> +	}
> +
>   	ret = component_bind_all(dev, drm);
>   	if (ret)
>   		return ret;

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH RESEND v4 v5 0/4] drm/vc4: Use the firmware to stop the display pipeline

[Maxime Ripard]

On Wed, 15 Dec 2021 10:51:13 +0100, Maxime Ripard wrote:
> The VC4 driver has had limited support to disable the HDMI controllers and
> pixelvalves at boot if the firmware has enabled them.
> 
> However, this proved to be limited, and a bit unreliable so a new firmware
> command has been introduced some time ago to make it free all its resources and
> disable any display output it might have enabled.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH RESEND v4 v5 4/4] drm/vc4: Notify the firmware when DRM is in charge

[Maxime Ripard]

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

Hi Thomas,

On Tue, Jan 11, 2022 at 10:38:36AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 15.12.21 um 10:51 schrieb Maxime Ripard:
> > Once the call to drm_fb_helper_remove_conflicting_framebuffers() has
> > been made, simplefb has been unregistered and the KMS driver is entirely
> > in charge of the display.
> > 
> > Thus, we can notify the firmware it can free whatever resource it was
> > using to maintain simplefb functional.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/vc4_drv.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> > index 86c61ee120b7..a03053c8e22c 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.c
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> > @@ -37,6 +37,8 @@
> >   #include <drm/drm_fb_helper.h>
> >   #include <drm/drm_vblank.h>
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> >   #include "uapi/drm/vc4_drm.h"
> >   #include "vc4_drv.h"
> > @@ -215,6 +217,7 @@ static void vc4_match_add_drivers(struct device *dev,
> >   static int vc4_drm_bind(struct device *dev)
> >   {
> >   	struct platform_device *pdev = to_platform_device(dev);
> > +	struct rpi_firmware *firmware = NULL;
> >   	struct drm_device *drm;
> >   	struct vc4_dev *vc4;
> >   	struct device_node *node;
> > @@ -251,10 +254,29 @@ static int vc4_drm_bind(struct device *dev)
> >   	if (ret)
> >   		return ret;
> > +	node = of_find_compatible_node(NULL, NULL, "raspberrypi,bcm2835-firmware");
> > +	if (node) {
> > +		firmware = rpi_firmware_get(node);
> > +		of_node_put(node);
> > +
> > +		if (!firmware)
> > +			return -EPROBE_DEFER;
> > +	}
> > +
> 
> The code is
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for your review

> Just for my understanding:
> 
> You retrieve the firmware before killing simpledrm simply to keep the
> display on if it fails, right?

Exactly

> What's the possible error that would justify a retry (via EPROBE_DEFER)?

The firmware there is backed by a driver that might not have probed yet,
in which case we just want to retry later on

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]