From: Imre Deak <imre.deak@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: linux-fbdev@vger.kernel.org,
Kai-Heng Feng <kai.heng.feng@canonical.com>,
dri-devel@lists.freedesktop.org,
Thomas Zimmermann <tzimmermann@suse.de>,
Alex Deucher <alexander.deucher@amd.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy
Date: Sat, 7 Aug 2021 18:21:10 +0300 [thread overview]
Message-ID: <20210807152110.GA2390730@ideak-desk.fi.intel.com> (raw)
In-Reply-To: <YQsTWSGIgY00XEU7@phenom.ffwll.local>
On Thu, Aug 05, 2021 at 12:23:21AM +0200, Daniel Vetter wrote:
> On Mon, Aug 02, 2021 at 04:35:51PM +0300, Imre Deak wrote:
> > Atm the EFI FB driver gets a runtime PM reference for the associated GFX
> > PCI device during driver probing and releases it only when removing the
> > driver.
> >
> > When fbcon switches to the FB provided by the PCI device's driver (for
> > instance i915/drmfb), the EFI FB will get only unregistered without the
> > EFI FB driver getting unloaded, keeping the runtime PM reference
> > acquired during driver probing. This reference will prevent the PCI
> > driver from runtime suspending the device.
> >
> > Fix this by releasing the RPM reference from the EFI FB's destroy hook,
> > called when the FB gets unregistered.
> >
> > Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0")
> > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Patch looks good:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> But I've found a bunch of ordering issues here:
> - we should probably get the runtime pm reference _before_ we register the
> framebuffer. There's a race right now about there.
Yea, missed this will send a v2 moving it earlier.
> - the sysfs_remove_groups and framebuffer_release should also be moved
> into the destroy callback. This is more a leak type of situation.
Those sysfs entries belong to the efifb platform device, showing the
bootup screen_info.lfb_* info, not related to fb_info, so imo
efifb_remove() is the correct place to remove those. But yes, freeing
fb_info seems to belong to fb_destroy().
Atm, things will blow up when unbinding the efifb device after the efifb
framebuffer was unregistered while removing it as a conflicting FB
(since unregister_framebuffer() will be called twice), so that would
need to be solved as well. Maybe remove_conflicting_pci_framebuffers()
could unregister the platform device instead of only unregistering the
framebuffer, similarly to drm_aperture_detach_firmware(), but haven't
checked this in more detail.
> Cheers, Daniel
>
> > ---
> > drivers/video/fbdev/efifb.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 8ea8f079cde26..25cdea32b9633 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -47,6 +47,8 @@ static bool use_bgrt = true;
> > static bool request_mem_succeeded = false;
> > static u64 mem_flags = EFI_MEMORY_WC | EFI_MEMORY_UC;
> >
> > +static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the efifb */
> > +
> > static struct fb_var_screeninfo efifb_defined = {
> > .activate = FB_ACTIVATE_NOW,
> > .height = -1,
> > @@ -243,6 +245,9 @@ static inline void efifb_show_boot_graphics(struct fb_info *info) {}
> >
> > static void efifb_destroy(struct fb_info *info)
> > {
> > + if (efifb_pci_dev)
> > + pm_runtime_put(&efifb_pci_dev->dev);
> > +
> > if (info->screen_base) {
> > if (mem_flags & (EFI_MEMORY_UC | EFI_MEMORY_WC))
> > iounmap(info->screen_base);
> > @@ -333,7 +338,6 @@ ATTRIBUTE_GROUPS(efifb);
> >
> > static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
> >
> > -static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the efifb */
> > static struct resource *bar_resource;
> > static u64 bar_offset;
> >
> > @@ -603,8 +607,6 @@ static int efifb_remove(struct platform_device *pdev)
> > unregister_framebuffer(info);
> > sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
> > framebuffer_release(info);
> > - if (efifb_pci_dev)
> > - pm_runtime_put(&efifb_pci_dev->dev);
> >
> > return 0;
> > }
> > --
> > 2.27.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
next prev parent reply other threads:[~2021-08-07 15:21 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-02 13:35 [Intel-gfx] [PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy Imre Deak
2021-08-02 15:05 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2021-08-03 0:58 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-08-04 22:23 ` [Intel-gfx] [PATCH] " Daniel Vetter
2021-08-07 15:21 ` Imre Deak [this message]
2021-08-09 14:19 ` Daniel Vetter
2021-08-09 13:31 ` [Intel-gfx] [PATCH v2] " Imre Deak
2021-08-10 8:36 ` Imre Deak
2021-08-10 14:48 ` Alex Deucher
2021-08-10 15:20 ` Kai-Heng Feng
2021-08-13 13:46 ` Imre Deak
2021-08-09 14:49 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for fbdev/efifb: Release PCI device's runtime PM ref during FB destroy (rev2) Patchwork
2021-08-09 15:12 ` Imre Deak
2021-08-09 16:37 ` Vudum, Lakshminarayana
2021-08-09 16:10 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-08-09 17:23 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2021-08-09 17:57 ` Imre Deak
2021-08-09 18:32 ` Vudum, Lakshminarayana
2021-08-09 18:21 ` [Intel-gfx] ✓ Fi.CI.IGT: success " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210807152110.GA2390730@ideak-desk.fi.intel.com \
--to=imre.deak@intel.com \
--cc=alexander.deucher@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=kai.heng.feng@canonical.com \
--cc=linux-fbdev@vger.kernel.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).