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

[Maxime Ripard <maxime@cerno.tech>]

[-- 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 --]