Paul, On Mon, Sep 4, 2023 at 2:15 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote: > > Hi Douglas, > > Le vendredi 01 septembre 2023 à 16:41 -0700, Douglas Anderson a écrit : > > Based on grepping through the source code this driver appears to be > > missing a call to drm_atomic_helper_shutdown() at system shutdown > > time. Among other things, this means that if a panel is in use that > > it > > won't be cleanly powered off at system shutdown time. > > > > The fact that we should call drm_atomic_helper_shutdown() in the case > > of OS shutdown/restart comes straight out of the kernel doc "driver > > instance overview" in drm_drv.c. > > > > Since this driver uses the component model and shutdown happens at > > the > > base driver, we communicate whether we have to call > > drm_atomic_helper_shutdown() by seeing if drvdata is non-NULL. > > > > Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx> > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > > LGTM. > Acked-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx> Thanks for the Ack! Would you expect this patch to land through "drm-misc", or do you expect it to go through some other tree? Running: ./scripts/get_maintainer.pl --scm -f drivers/gpu/drm/ingenic/ingenic-drm-drv.c ...does not show that this driver normally goes through drm-misc, but it also doesn't show that it goes through any other tree so maybe it's just an artifact of the way it's tagged in the MAINTAINERS file? If it's fine for this to go through drm-misc, I'll probably land it (with your Ack and Maxime's Review) sooner rather than later just to make this patch series less unwieldy. > > --- > > This commit is only compile-time tested. > > > > NOTE: this patch touches a lot more than other similar patches since > > the bind() function is long and we want to make sure that we unset > > the > > drvdata if bind() fails. > > > > While making this patch, I noticed that the bind() function of this > > driver is using "devm" and thus assumes it doesn't need to do much > > explicit error handling. That's actually a bug. As per kernel docs > > [1] > > "the lifetime of the aggregate driver does not align with any of the > > underlying struct device instances. Therefore devm cannot be used and > > all resources acquired or allocated in this callback must be > > explicitly released in the unbind callback". Fixing that is outside > > the scope of this commit. > > > > [1] https://docs.kernel.org/driver-api/component.html > > > > Noted, thanks. FWIW, I think that at least a few other DRM drivers handle this by doing some of their resource allocation / acquiring in the probe() function and then only doing things in the bind() that absolutely need to be in the bind. ;-) -Doug
