Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper

[Doug Anderson]

Hi,

On Tue, Aug 29, 2023 at 1:38 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> On Mon, Aug 28, 2023 at 09:06:29AM -0700, Doug Anderson wrote:
> > > > Shutdown is called any time you reboot a device. That means that if a
> > > > DRM driver is _not_ calling drm_atomic_helper_shutdown() on the
> > > > panel's behalf at shutdown time then the panel won't be powered off
> > > > properly. This feels to me like something that might actually matter.
> > >
> > > It does matter. What I disagree on is that you suggest working around
> > > that brokenness in the core framework. What I'm saying is driver is
> > > broken, we should keep the core framework sane and fix it in the driver.
> > >
> > > It should be fairly easy with a coccinelle script to figure out which
> > > panels are affected, and to add that call in remove.
> >
> > I think I'm confused here. I've already figured out which panels are
> > affected in my patch series, right? It's the set of panels that today
> > try to power the panel off in their shutdown call, right? ...but I
> > think we can't add the call you're suggesting,
> > drm_atomic_helper_shutdown(), to the _panel_'s shutdown callback, can
> > we? We need to add it to the shutdown callback of the top-level DRM
> > driver, right?
>
> I have no idea what happens if we just unbind the panel device from its
> driver.
>
> If we can't, then it's all fine. If we can, then we need figure out how
> to unregister the DRM device (or block the unbinding from happening).

Nothing prevents unbinding the panel driver directly. I just confirmed
on my sc7180-lazor:

cd /sys/bus/dp-aux/drivers/panel-simple-dp-aux
echo aux-2-0008 > unbind

...no errors happened and the panel unbound. I think this is as
expected since I'm not aware of a way to prevent unbinding a driver. I
think the relevant function is unbind_store() in bus.c, right? There
is no failure code returned by device_driver_detach(). Presumably this
is by design?

FWIW, also trying to re-bind didn't work, also as expected (I think).
I think the whole bridge chain would need to be resolved again and
nothing I'm aware of makes that happen. Maybe I'm simply missing
something in my understanding, of course.

If you want to attempt to tackle some of these issues then I'd be more
than happy. I'm already waist deep in yak hair and I think this is too
big of a task for me to add on.

Should this issue block my work, then? Today, panels will at least
cleanly power themselves off if someone tries to unbind them like
that. If I remove the clean power off at driver unbind time and rely
on the DRM driver to power panels off cleanly then if someone directly
unbinds a panel like I've done above then it won't be power sequenced
properly, right?


> > > > Panels tend to be one of those things that really care about their
> > > > power sequencing and can even get damaged (or not turn on properly
> > > > next time) if sequencing is not done properly, so just removing this
> > > > code and putting the blame on the DRM driver seems scary to me.
> > >
> > > Sure, it's bad. But there's no difference compared to the approach you
> > > suggest in that patch: you created a helper, yes, but every driver will
> > > still have to call that helper and if they don't, the panel will still
> > > be called and it's a bug. And we would have to convert everything to
> > > that new helper.
> > >
> > > It's fundamentally the same discussion than what you were opposed to
> > > above.
> >
> > I think the key difference here is that, if I understand correctly,
> > drm_atomic_helper_shutdown() needs to be added to the top-level DRM
> > driver, not to the panel itself. I guess I'm worried that I'll either
> > miss a case or that simply adding a call to
> > drm_atomic_helper_shutdown() in the top-level DRM driver will break
> > something. Well, I suppose I can try it and see what happens...
>
> The more I think about this discussion, the more I think that the
> original intent of the prepared/enabled flags were precisely there to
> prevent a double-disable for drivers with drm_atomic_helper_shutdown(),
> while still shutting down the panel resources when the panel is used
> with a driver that doesn't call it.

Sure, that makes sense.


> Honestly, I think the right thing to do here is to make every driver
> call shutdown, and then you don't need the reference counting anymore.
>
> I had a shot at a (possibly very suboptimal) coccinelle script to look
> for drivers that are KMS drivers but don't call
> drm_atomic_helper_shutdown() at shutdown.
>
> https://paste.ack.tf/bb42e6@raw

Your paste seems to have expired. Maybe you used the default and had
it expire in 1 day? Maybe you could just paste the script in email so
it'll be archived for posterity on lore?


> The result is:
>
> $ make coccicheck COCCI=./test-drm-shutdown.cocci MODE=report
>
> ...
>
> ./drivers/gpu/drm/sti/sti_drv.c:262:30-49: ERROR: KMS driver sti_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/armada/armada_drv.c:245:30-56: ERROR: KMS driver armada_drm_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1637:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/vboxvideo/vbox_drv.c:163:25-40: ERROR: KMS driver vbox_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/tiny/arcpgu.c:424:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/omapdrm/omap_drv.c:856:30-34: ERROR: KMS driver pdev is missing shutdown implementation
> ./drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c:361:30-57: ERROR: KMS driver fsl_dcu_drm_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/tve200/tve200_drv.c:261:30-43: ERROR: KMS driver tve200_driver is missing shutdown implementation
> ./drivers/gpu/drm/stm/drv.c:235:30-53: ERROR: KMS driver stm_drm_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/qxl/qxl_drv.c:267:25-39: ERROR: KMS driver qxl_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/logicvc/logicvc_drm.c:494:30-57: ERROR: KMS driver logicvc_drm_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/sun4i/sun4i_drv.c:439:30-55: ERROR: KMS driver sun4i_drv_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c:827:30-60: ERROR: KMS driver atmel_hlcdc_dc_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/mcde/mcde_drv.c:471:30-41: ERROR: KMS driver mcde_driver is missing shutdown implementation
> ./drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c:366:25-41: ERROR: KMS driver hibmc_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/aspeed/aspeed_gfx_drv.c:365:30-56: ERROR: KMS driver aspeed_gfx_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/ingenic/ingenic-drm-drv.c:1607:30-48: ERROR: KMS driver ingenic_drm_driver is missing shutdown implementation
> ./drivers/gpu/drm/arm/malidp_drv.c:982:30-52: ERROR: KMS driver malidp_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/arm/hdlcd_drv.c:400:30-51: ERROR: KMS driver hdlcd_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/kmb/kmb_drv.c:622:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/exynos/exynos_drm_drv.c:356:30-56: ERROR: KMS driver exynos_drm_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/tiny/bochs.c:718:25-41: ERROR: KMS driver bochs_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/tiny/cirrus.c:746:25-42: ERROR: KMS driver cirrus_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/mediatek/mtk_drm_drv.c:954:30-53: ERROR: KMS driver mtk_drm_platform_driver is missing shutdown implementation
>
> It's a significant number of drivers, but it's not the end of the world,
> really.

My own analysis shows more than that, actually. I can't look at your
script since it's expired, but as one example your list seems to have
neither imx/ipuv3 nor imx/dcss. imx/ipuv3 seems to be missing
drm_atomic_helper_shutdown() at both unbind and system shutdown time.
imx/dcss seems to be missing drm_atomic_helper_shutdown() at system
shutdown time, though it does have it at driver remove time. Did I
mess up in identifying those two drivers as ones that need fixing?

I can't give you an exact list because I don't have a great search
that identifies the problem. I'm mostly looking for all instances of
drm_dev_register() where the driver is "DRIVER_MODESET" and then
manually checking their use of drm_atomic_helper_shutdown(). Until I
get through them all I won't know the count, and it's a manual
process.


-Doug