<div dir="ltr"> On Wed, Jul 15, 2015 at 2:34 AM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>> 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, <a href="mailto:jay.p.patel@intel.com">jay.p.patel@intel.com</a> wrote: > > > > From: Jay Patel <<a href="mailto:jay.p.patel@intel.com">jay.p.patel@intel.com</a>> > > > > > > > > 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 <<a href="mailto:jay.p.patel@intel.com">jay.p.patel@intel.com</a>> > > > > --- > > > > 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 <<a href="mailto:tiwai@suse.de">tiwai@suse.de</a>> > 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 > > > > > > <a href="http://www.gossamer-threads.com/lists/linux/kernel/2010793">http://www.gossamer-threads.com/lists/linux/kernel/2010793</a> > > > > > > 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.<div> </div><div>Finally got some time to chase this down - it's not a technical limitation</div><div>(ChromeOS does use udev) - it's a violation of the security model. With direct</div><div>kernel loading of firmware, checks can happen that ensure the firmware is</div><div>coming from the dm-verity RO-mounted root fs. If a userspace process is</div><div>providing the firmware through the sysfs entry, there's no way to verify that</div><div>the firmware is coming from a trusted file/partition, as the kernel has no</div><div>knowledge of the source of the incoming data.</div><div> </div><div>-James</div><div> </div><div> > > > > Acked-by: Daniel Vetter <<a href="mailto:daniel.vetter@ffwll.ch">daniel.vetter@ffwll.ch</a>> > > > > > > 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 > -- > Daniel Vetter > Software Engineer, Intel Corporation > <a href="http://blog.ffwll.ch">http://blog.ffwll.ch</a> > _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> -- James Ausmus ChromeOS Integration Architect SSG-OTC ChromeOS Integration</div></div>