[RFC 0/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[RFC 0/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[jay.p.patel]

From: jay <jay.p.patel@intel.com>

Chrome OS is "noinitrd" OS. It does not normally allow in-built firmware in kernel.
i915 driver initialization precedes the initialization of file system. Hence, loading
fails whenever driver tries to load CSR firmware from file system.

The current implementation uses "request_firmware_nowait()" function which creates
an asynchronous thread running concurrently with the rest of the system initialization.
However it tries to load firmware only once and fails immediately if file does not
exist. The following link discusses the related issue.
(http://thread.gmane.org/gmane.linux.kernel/1787377/focus=1791177)

This patch is an interim solution which is targeted towards Chrome OS/Android and
to be used until a long term solution is available. request_firmware() is called
in a worker thread which initially waits for file system to be initialized and then
loads the firmware.

This patch is not a merge candidate but its targets an instigation of discussion on
sustainable near term solution for the problem.

Thanks,
Jay

Jay Patel (1):
  drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

 drivers/gpu/drm/i915/i915_drv.c  |  2 ++
 drivers/gpu/drm/i915/intel_csr.c | 58 ++++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 11 deletions(-)

-- 
2.1.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[jay.p.patel]

From: Jay Patel <jay.p.patel@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.

"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 <jay.p.patel@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);
+	}
+
+	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

Re: [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[Daniel Vetter]

On Mon, Jul 13, 2015 at 09:36:45AM -0700, jay.p.patel@intel.com wrote:
> From: Jay Patel <jay.p.patel@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
> Signed-off-by: Jay Patel <jay.p.patel@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).

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 <daniel.vetter@ffwll.ch>

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[Greg KH]

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 <jay.p.patel@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@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.

> 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.

> 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?

> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Also adding Greg so he knows what's happening here.

Ick, please don't take this as-is.

greg k-h
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[Daniel Vetter]

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 <jay.p.patel@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@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@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.

> > Acked-by: Daniel Vetter <daniel.vetter@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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[Ausmus, James]

[-- Attachment #1.1: Type: text/plain, Size: 8793 bytes --]

On Wed, Jul 15, 2015 at 2:34 AM, Daniel Vetter <daniel@ffwll.ch> 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 <jay.p.patel@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@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@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.

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@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
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx




--


James Ausmus
ChromeOS Integration Architect
SSG-OTC ChromeOS Integration

[-- Attachment #1.2: Type: text/html, Size: 11747 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/1] drm/i915 : Wait until SYSTEM_RUNNING before loading CSR firmware

[James Ausmus]

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 <jay.p.patel@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@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@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@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx