Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753393AbbGNJT4 (ORCPT ); Tue, 14 Jul 2015 05:19:56 -0400 Received: from mail-wg0-f53.google.com ([74.125.82.53]:36374 "EHLO mail-wg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751209AbbGNJTx (ORCPT ); Tue, 14 Jul 2015 05:19:53 -0400 Date: Tue, 14 Jul 2015 11:22:35 +0200 From: Daniel Vetter To: jay.p.patel@intel.com Cc: intel-gfx@lists.freedesktop.org, Greg KH , LKML Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware Message-ID: <20150714092235.GL3736@phenom.ffwll.local> Mail-Followup-To: jay.p.patel@intel.com, intel-gfx@lists.freedesktop.org, Greg KH , LKML References: <1436805405-1134-1-git-send-email-jay.p.patel@intel.com> <1436805405-1134-2-git-send-email-jay.p.patel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436805405-1134-2-git-send-email-jay.p.patel@intel.com> X-Operating-System: Linux phenom 4.2.0-rc1+ User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6316 Lines: 172 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 > 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). 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. 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 Acked-by: Daniel Vetter Also adding Greg so he knows what's happening here. -Daniel > + > + ret = request_firmware(&fw, csr->fw_path, &dev_priv->dev->pdev->dev); > + if (ret) { > + i915_firmware_load_error_print(csr->fw_path, ret); > + intel_csr_load_status_set(dev_priv, FW_FAILED); > + } else { > + finish_csr_load(fw, dev_priv); > + put_device(&dev_priv->dev->pdev->dev); > + module_put(fw_work->module); > + } > + > + kfree(fw_work); > +} > + > void intel_csr_ucode_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_csr *csr = &dev_priv->csr; > - int ret; > + struct csr_firmware_work *fw_work; > > if (!HAS_CSR(dev)) > return; > @@ -382,15 +417,16 @@ void intel_csr_ucode_init(struct drm_device *dev) > */ > intel_runtime_pm_get(dev_priv); > > - /* CSR supported for platform, load firmware */ > - ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path, > - &dev_priv->dev->pdev->dev, > - GFP_KERNEL, dev_priv, > - finish_csr_load); > - if (ret) { > - i915_firmware_load_error_print(csr->fw_path, ret); > - intel_csr_load_status_set(dev_priv, FW_FAILED); > - } > + /* Creating a thread which loads firmware */ > + fw_work = kzalloc(sizeof(struct csr_firmware_work), GFP_KERNEL); > + if (!fw_work) { > + return; > + } > + fw_work->module = THIS_MODULE; > + fw_work->dev = dev; > + INIT_WORK(&fw_work->work, csr_firmware_work_func); > + schedule_work(&fw_work->work); > + DRM_DEBUG("CSR firmware loader thread started \n"); > } > > void intel_csr_ucode_fini(struct drm_device *dev) > -- > 2.1.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- 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/