[Intel-gfx] [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware
James Ausmus
james.ausmus at intel.com
Wed Aug 12 19:51:35 PDT 2015
On Wednesday 15 July 2015 11:34:43 Daniel Vetter wrote:
> On Tue, Jul 14, 2015 at 01:37:32PM -0700, Greg KH wrote:
> > On Tue, Jul 14, 2015 at 11:22:35AM +0200, Daniel Vetter wrote:
> > > On Mon, Jul 13, 2015 at 09:36:45AM -0700, jay.p.patel at intel.com wrote:
> > > > From: Jay Patel <jay.p.patel at intel.com>
> > > >
> > > > NOTE: This is an interim solution which is targeted towards
> > > > Chrome OS/Android to be used until a long term solution is available.
> > > >
> > > > In this patch, request_firmware() is called in a worker thread
> > > > which initially waits for file system to be initialized and then
> > > > attempts to load the firmware.
> > >
> > > Aside: I posted a bunch of proof-of-principle patches to clean up dmc
> > > loading and convert over to using an explicit workqueue. They're being
> > > tested/made-to-actually-work right now I think.
> > >
> > > > "request_firmware_nowait()" is also using an asynchronous thread
> > > > running concurrently with the rest of the system initialization.
> > > > However, it tries to load firmware only once without checking the
> > > > sytem status and fails most of the time.
> > > >
> > > > Change-Id: I2cb16a768e54a85f48a6682d9690b4c8af844668
> >
> > What's this line for? :)
> >
> > > > Signed-off-by: Jay Patel <jay.p.patel at intel.com>
> > > > ---
> > > >
> > > > drivers/gpu/drm/i915/i915_drv.c | 2 ++
> > > > drivers/gpu/drm/i915/intel_csr.c | 58
> > > > ++++++++++++++++++++++++++++++++-------- 2 files changed, 49
> > > > insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c index 8c8407d..eb6f7e3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -559,6 +559,7 @@ void intel_hpd_cancel_work(struct drm_i915_private
> > > > *dev_priv)> > >
> > > > void i915_firmware_load_error_print(const char *fw_path, int err)
> > > > {
> > > >
> > > > DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
> > > >
> > > > + DRM_ERROR("The firmware file may be missing\n");
> > > >
> > > > /*
> > > >
> > > > * If the reason is not known assume -ENOENT since that's the most
> > > >
> > > > @@ -574,6 +575,7 @@ void i915_firmware_load_error_print(const char
> > > > *fw_path, int err)> > >
> > > > "The driver is built-in, so to load the firmware you need to\n"
> > > > "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n"
> > > > "in your initrd/initramfs image.\n");
> > > >
> > > > +
> > > >
> > > > }
> > > >
> > > > static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c
> > > > b/drivers/gpu/drm/i915/intel_csr.c index 9311cdd..8d1f08c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > > @@ -349,7 +349,7 @@ static void finish_csr_load(const struct firmware
> > > > *fw, void *context)> > >
> > > > /* load csr program during system boot, as needed for DC states */
> > > > intel_csr_load_program(dev);
> > > > fw_loaded = true;
> > > >
> > > > -
> > > > + DRM_INFO("CSR Firmware Loaded\n");
> > > >
> > > > out:
> > > > if (fw_loaded)
> > > >
> > > > intel_runtime_pm_put(dev_priv);
> > > >
> > > > @@ -359,11 +359,46 @@ out:
> > > > release_firmware(fw);
> > > >
> > > > }
> > > >
> > > > +struct csr_firmware_work {
> > > > + struct work_struct work;
> > > > + struct module *module;
> > > > + struct drm_device *dev;
> > > > +};
> > > > +
> > > > +/* csr_firmware_work_func() - thread function for loading the
> > > > firmware*/
> > > > +static void csr_firmware_work_func(struct work_struct *work)
> > > > +{
> > > > + const struct firmware *fw;
> > > > + const struct csr_firmware_work *fw_work = container_of(work,
struct
> > > > csr_firmware_work, work); + int ret;
> > > > + struct drm_device *dev = fw_work->dev;
> > > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > > + struct intel_csr *csr = &dev_priv->csr;
> > > > +
> > > > + /* Wait until root filesystem is loaded in case the firmware
> > > > + * is not built-in but in /lib/firmware */
> > > > + while(system_state != SYSTEM_RUNNING){
> > > > + msleep(500);
> > > > + }
> > >
> > > Yeah, not going to merge that right now until we've had a decent
> > > discussion with Greg KH (since imo his stance of every driver creating
> > > it's own retry loop just doesn't work, especially not with gfx where
> > > init
> > > is hairy and you just don't want to retry without end).
> >
> > Exactly, this type of thing isn't good at all (especially given that
> > the code isn't even checkpatch clean...)
> >
> > Don't do this. If you really want to somehow handle built-in drivers
> > that need firmware before the root filesystem is present, then use the
> > async firmware loading interface, don't sit and spin, that's crazy.
>
> This code is called from a work queue already to facilitate async loading.
> I want an explicit work queue so that we properly sync with it everywhere
> like driver unload or resume (otherwise we need a completion or
> something). And with an explicit worker I can put the entire init sequence
> for that component of the gpu in there, which means whether we require
> firmware or no doesn't change how the driver is loaded. Unified driver
> load paths is a fairly strict requirement I have (because otherwise
> testing is nigh impossible due to combinatorial explosion). I also don't
> want to ever reattempt loading the firmware since those kind of fallback
> paths are equally horrible from a testing perspective. If fw loading fails
> for some reason we'll just move on and declare that particular gpu part
> dead/unsupported.
>
> The other issue with request_firmware_nowait is that it doesn't do the
> uevent + udev fallback afaiui, see
>
> commit 5a1379e8748a5cfa3eb068f812d61bde849ef76c
> Author: Takashi Iwai <tiwai at suse.de>
> Date: Wed Jun 4 17:48:15 2014 +0200
>
> firmware loader: allow disabling of udev as firmware loader
>
> Only request_firmware seems to do that combo.
>
> > > Aside: Another solution might be the wait_for_rootfs from
> > >
> > > http://www.gossamer-threads.com/lists/linux/kernel/2010793
> > >
> > > But if Greg insists that each driver needs to solve this themselves then
> > > I'll pull something like this into upstream, but probably with a Kconfig
> > > option to disable it for normal linux userspace.
> >
> > "solve" this by just not sitting and spining, wait for userspace to load
> > your firmware if it needs it. Or, even better yet, if you really need
> > firmare at early boot before a rootfs, build the firmware into the
> > kernel image, like we used to do for a few decades.
>
> That's exactly what this tries to do (not in a terribly pretty way I
> admit).
>
> And building the firmware into the image isn't an option since that seems
> to freak out legal or something like that. And loading modules really
> early in initrd (like it's done on desktop linux distros) is also not
> something since for a pile of reasons cros/android want monolithic kernel
> images.
>
> > > The other option would
> > > be to use CONFIG_FW_LOADER_USERSPACE_FALLBACK udev helper. That might be
> > > an option for intel android, but it sounds like not something cros wants
> > > to do. Therefore
> >
> > why would chromeos not want to use the udev helper?
>
> I'm trying to sell them on it and haven't yet figured out why it's not ok,
> but it seems to be a popular request. Other folks also came up with
> similar hacks (the wait_for_rootfs one linked above) so I'm assume it's
> not entirely context free. On these machines everything is static making a
> lot of hotplug processing unecessary.
(Resending without the gmail-forced html content-type)
Finally got some time to chase this down - it's not a technical limitation
(ChromeOS does use udev) - it's a violation of the security model. With direct
kernel loading of firmware, checks can happen that ensure the firmware is coming
from the dm-verity RO-mounted root fs. If a userspace process is providing the
firmware through the sysfs entry, there's no way to verify that the firmware is
coming from a trusted file/partition, as the kernel has no knowledge of the
source of the incoming data.
-James
>
> > > Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > >
> > > Also adding Greg so he knows what's happening here.
> >
> > Ick, please don't take this as-is.
>
> Well I'd prefer if request_firmware just handles this for me since it
> seems to be a general need. But I'm ok with carrying this around in i915
> only too.
> -Daniel
More information about the Intel-gfx
mailing list