Hi,
On 2023/9/15 21:44, Doug Anderson wrote:
Hi,
On Fri, Sep 15, 2023 at 2:11 AM suijingfeng <suijingfeng@xxxxxxxxxxx> wrote:
Hi,
On 2023/9/2 07:39, Douglas Anderson wrote:
Based on grepping through the source code these drivers appear 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.
All of the drivers in this patch were fairly straightforward to fix
since they already had a call to drm_atomic_helper_shutdown() at
remove/unbind time but were just lacking one at system shutdown. The
only hitch is that some of these drivers use the component model to
register/unregister their DRM devices. The shutdown callback is part
of the original device. The typical solution here, based on how other
DRM drivers do this, is to keep track of whether the device is bound
based on drvdata. In most cases the drvdata is the drm_device, so we
can just make sure it is NULL when the device is not bound. In some
drivers, this required minor code changes. To make things simpler,
drm_atomic_helper_shutdown() has been modified to consider a NULL
drm_device as a noop in the patch ("drm/atomic-helper:
drm_atomic_helper_shutdown(NULL) should be a noop").
Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx>
Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
---
I have just tested the whole series, thanks for the patch. For drm/loongson only:
Reviewed-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
Tested-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
Thanks!
By the way, I add 'pr_info("lsdc_pci_shutdown\n");' into the lsdc_pci_shutdown() function,
And seeing that lsdc_pci_shutdown() will be called when reboot and shutdown the machine.
I did not witness something weird happen at present. As you have said, this is useful for
drm panels drivers. But for the rest(drm/hibmc, drm/ast, drm/mgag200 and drm/loongson etc)
drivers, you didn't mention what's the benefit for those drivers.
I didn't mention it because I have no idea! I presume that for
non-drm_panel use cases it's not a huge deal, otherwise it wouldn't
have been missing from so many drivers. Thus, my "one sentence" reason
for the non-drm_panel case is just "we should do this because the
documentation of the API says we should", which is already in the
commit message. ;-)
OK, this sound fine.
If you have a specific other benefit you'd like me to list then I'm happy to.
You should think about the answer of this question yourself.
But I will not object if you can't find one. OK. :-)
Probably, you can
mention it with at least one sentence at the next version. I also prefer to alter the
lsdc_pci_shutdown() function as the following pattern:
static void lsdc_pci_shutdown(struct pci_dev *pdev)
{
struct drm_device *ddev = pci_get_drvdata(pdev);
drm_atomic_helper_shutdown(ddev);
}
I was hoping to land this patch without spinning it unless there's a
good reason. How strongly do you feel about needing to change the
above?
Not very strong, this version looks just fine.
I will not object if you keep it as is.
But I will also hear what the others reviewers say.
Thanks for the patch.
-Doug