Re: [PATCH v2 3/3] drm/vc4: Notify the firmware when DRM is in charge

From: nicolas saenz julienne <nsaenz@kernel.org>
To: Maxime Ripard <maxime@cerno.tech>, Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	 Florian Fainelli <f.fainelli@gmail.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	David Airlie <airlied@linux.ie>
Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>,
	Phil Elwell <phil@raspberrypi.com>,
	Tim Gover <tim.gover@raspberrypi.com>,
	Dom Cobley <dom@raspberrypi.com>,
	linux-rpi-kernel@lists.infradead.org,
	 devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	 dri-devel@lists.freedesktop.org,
	bcm-kernel-feedback-list@broadcom.com
Subject: Re: [PATCH v2 3/3] drm/vc4: Notify the firmware when DRM is in charge
Date: Fri, 10 Dec 2021 10:55:12 +0100	[thread overview]
Message-ID: <a2c1d56378748b037a8d62dcc2934c4f8eabf5b2.camel@kernel.org> (raw)
In-Reply-To: <20211203135126.700165-4-maxime@cerno.tech>

Hi Maxime,

On Fri, 2021-12-03 at 14:51 +0100, Maxime Ripard 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>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.c | 19 +++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_drv.h |  2 ++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 8ab89f805826..38d55a47c831 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"
> @@ -251,10 +253,27 @@ static int vc4_drm_bind(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	node = of_find_compatible_node(NULL, NULL, "raspberrypi,bcm2835-firmware");
> +	if (node) {
> +		vc4->firmware = devm_rpi_firmware_get(dev, node);

I'm really sorry for contradicting myself, but I think it makes more sense to
do rpi_firmware_get() here...

> +		of_node_put(node);
> +
> +		if (!vc4->firmware)
> +			return -EPROBE_DEFER;
> +	}
> +
>  	ret = drm_aperture_remove_framebuffers(false, &vc4_drm_driver);
>  	if (ret)
>  		return ret;
>  
> +	if (vc4->firmware) {
> +		ret = rpi_firmware_property(vc4->firmware,
> +					    RPI_FIRMWARE_NOTIFY_DISPLAY_DONE,
> +					    NULL, 0);
> +		if (ret)
> +			drm_warn(drm, "Couldn't stop firmware display driver: %d\n", ret);
> +	}
> +

...and rpi_firmware_put() here. IIUC after this the fw handle isn't needed
anymore.

Other than that, the series looks fine to me.

Regards,
Nicolas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel