Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751911AbbHMC7N (ORCPT ); Wed, 12 Aug 2015 22:59:13 -0400 Received: from mga09.intel.com ([134.134.136.24]:63480 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751255AbbHMC7M (ORCPT ); Wed, 12 Aug 2015 22:59:12 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,666,1432623600"; d="scan'208";a="747528274" From: James Ausmus To: intel-gfx@lists.freedesktop.org Cc: Daniel Vetter , Greg KH , LKML , Joe Konno Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware Date: Wed, 12 Aug 2015 19:51:35 -0700 Message-ID: <1538357.xZgglP0WZF@jausmus-gentoo-dev4> Organization: Intel User-Agent: KMail/4.14.10 (Linux/4.1.4-gentoo; KDE/4.14.10; x86_64; ; ) In-Reply-To: <20150715093443.GG3736@phenom.ffwll.local> References: <1436805405-1134-1-git-send-email-jay.p.patel@intel.com> <20150714203732.GA20241@kroah.com> <20150715093443.GG3736@phenom.ffwll.local> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8959 Lines: 212 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@intel.com wrote: > > > > From: Jay Patel > > > > > > > > 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 > > > > --- > > > > > > > > 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 > 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 > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/