Skip to contentSkip to conversation

drm/msm/adreno: load gpu at probe/bind time

PATCH In review Rob Clark 2 participants 5 messages Oct 16, 2017 1 message not in the archive

Subsystems (4)
lore
Share & tools
lore

Rob Clark

author
#0 lore raw

PATCH 1/3 drm/msm/adreno: load gpu at probe/bind time

Previously, in an effort to defer initializing the gpu until firmware was available (ie. rootfs mounted), the gpu was not loaded at when the subdevice was bound. Which resulted that clks/etc were requested in a place that devm couldn't really help unwind if something failed.

Instead move request_firmware() to gpu->hw_init() and construct the gpu earlier in adreno_bind(). To avoid the rest of the driver needing to be aware of a gpu that hasn't managed to load firmware and hw_init() yet, stash the gpu ptr in the adreno device's drvdata, and don't set priv->gpu() until hw_init() succeeds.

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  6 +--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 78 +++++++++++++++---------------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 45 +++++++++++------
 drivers/gpu/drm/msm/msm_drv.c              | 10 ----
 4 files changed, 74 insertions(+), 65 deletions(-)
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +3 -3
index 17c59d839e6f..8df86d83d859 100644
@@ -536,13 +536,13 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 		REG_A5XX_RBBM_SECVID_TSB_TRUSTED_BASE_HI, 0x00000000);
 	gpu_write(gpu, REG_A5XX_RBBM_SECVID_TSB_TRUSTED_SIZE, 0x00000000);
 
-	/* Load the GPMU firmware before starting the HW init */
-	a5xx_gpmu_ucode_init(gpu);
-
 	ret = adreno_hw_init(gpu);
 	if (ret)
 		return ret;
 
+	/* Load the GPMU firmware before starting the HW init */
+	a5xx_gpmu_ucode_init(gpu);
+
 	ret = a5xx_ucode_init(gpu);
 	if (ret)
 		return ret;
drivers/gpu/drm/msm/adreno/adreno_device.c +40 -38

110 lines not on this page · raw message

drivers/gpu/drm/msm/adreno/adreno_gpu.c +31 -14

63 lines not on this page · raw message

drivers/gpu/drm/msm/msm_drv.c +0 -10
index 606df7bea97b..d4d4146b3425 100644
@@ -211,7 +211,6 @@ static int msm_drm_uninit(struct device *dev)
 	struct drm_device *ddev = platform_get_drvdata(pdev);
 	struct msm_drm_private *priv = ddev->dev_private;
 	struct msm_kms *kms = priv->kms;
-	struct msm_gpu *gpu = priv->gpu;
 	struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl;
 	struct vblank_event *vbl_ev, *tmp;
 
@@ -253,15 +252,6 @@ static int msm_drm_uninit(struct device *dev)
 	if (kms && kms->funcs)
 		kms->funcs->destroy(kms);
 
-	if (gpu) {
-		mutex_lock(&ddev->struct_mutex);
-		// XXX what do we do here?
-		//pm_runtime_enable(&pdev->dev);
-		gpu->funcs->pm_suspend(gpu);
-		mutex_unlock(&ddev->struct_mutex);
-		gpu->funcs->destroy(gpu);
-	}
-
 	if (priv->vram.paddr) {
 		unsigned long attrs = DMA_ATTR_NO_KERNEL_MAPPING;
 		drm_mm_takedown(&priv->vram.mm);
Signature · 1 line
2.13.6
author
#1 lore raw

PATCH 2/3 drm/msm/adreno: split out helper to load fw

Prep work for the next patch.

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 11 ++++++-----
 drivers/gpu/drm/msm/adreno/a5xx_power.c |  3 ++-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 35 ++++++++++++++++++++++-----------
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  2 ++
 4 files changed, 33 insertions(+), 18 deletions(-)
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +6 -5
index 8df86d83d859..4693659aba2e 100644
@@ -26,8 +26,9 @@ static void a5xx_dump(struct msm_gpu *gpu);
 
 #define GPU_PAS_ID 13
 
-static int zap_shader_load_mdt(struct device *dev, const char *fwname)
+static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname)
 {
+	struct device *dev = &gpu->pdev->dev;
 	const struct firmware *fw;
 	struct device_node *np;
 	struct resource r;
@@ -55,10 +56,10 @@ static int zap_shader_load_mdt(struct device *dev, const char *fwname)
 	mem_size = resource_size(&r);
 
 	/* Request the MDT file for the firmware */
-	ret = request_firmware(&fw, fwname, dev);
-	if (ret) {
+	fw = adreno_request_fw(to_adreno_gpu(gpu), fwname);
+	if (IS_ERR(fw)) {
 		DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname);
-		return ret;
+		return PTR_ERR(fw);
 	}
 
 	/* Figure out how much memory we need */
@@ -381,7 +382,7 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 		return -ENODEV;
 	}
 
-	ret = zap_shader_load_mdt(&pdev->dev, adreno_gpu->info->zapfw);
+	ret = zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw);
 
 	loaded = !ret;
 
drivers/gpu/drm/msm/adreno/a5xx_power.c +2 -1
index 04aab1dcae2b..b5de2be67732 100644
@@ -264,7 +264,8 @@ void a5xx_gpmu_ucode_init(struct msm_gpu *gpu)
 		return;
 
 	/* Get the firmware */
-	if (request_firmware(&fw, adreno_gpu->info->gpmufw, drm->dev)) {
+	fw = adreno_request_fw(adreno_gpu, adreno_gpu->info->gpmufw);
+	if (IS_ERR(fw)) {
 		DRM_ERROR("%s: Could not get GPMU firmware. GPMU will not be active\n",
 			gpu->name);
 		return;
drivers/gpu/drm/msm/adreno/adreno_gpu.c +23 -12
index 8caa7f1f012d..19e12380297e 100644
@@ -64,28 +64,39 @@ int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
 	}
 }
 
-static int adreno_load_fw(struct adreno_gpu *adreno_gpu)
+const struct firmware *
+adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname)
 {
 	struct drm_device *drm = adreno_gpu->base.dev;
+	const struct firmware *fw = NULL;
 	int ret;
 
+	ret = request_firmware(&fw, fwname, drm->dev);
+	if (ret) {
+		dev_err(drm->dev, "failed to load %s: %d\n", fwname, ret);
+		return ERR_PTR(ret);
+	}
+
+	return fw;
+}
+
+static int adreno_load_fw(struct adreno_gpu *adreno_gpu)
+{
+	const struct firmware *fw;
+
 	if (adreno_gpu->pm4)
 		return 0;
 
-	ret = request_firmware(&adreno_gpu->pm4, adreno_gpu->info->pm4fw, drm->dev);
-	if (ret) {
-		dev_err(drm->dev, "failed to load %s PM4 firmware: %d\n",
-				adreno_gpu->info->pm4fw, ret);
-		return ret;
-	}
+	fw = adreno_request_fw(adreno_gpu, adreno_gpu->info->pm4fw);
+	if (IS_ERR(fw))
+		return PTR_ERR(fw);
+	adreno_gpu->pm4 = fw;
 
-	ret = request_firmware(&adreno_gpu->pfp, adreno_gpu->info->pfpfw, drm->dev);
-	if (ret) {
-		dev_err(drm->dev, "failed to load %s PFP firmware: %d\n",
-				adreno_gpu->info->pfpfw, ret);
+	fw = adreno_request_fw(adreno_gpu, adreno_gpu->info->pfpfw);
+	if (IS_ERR(fw)) {
 		release_firmware(adreno_gpu->pm4);
 		adreno_gpu->pm4 = NULL;
-		return ret;
+		return PTR_ERR(fw);
 	}
 
 	return 0;
drivers/gpu/drm/msm/adreno/adreno_gpu.h +2 -0
index 4d9165f29f43..3eda98af546e 100644
@@ -196,6 +196,8 @@ static inline int adreno_is_a530(struct adreno_gpu *gpu)
 }
 
 int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value);
+const struct firmware *adreno_request_fw(struct adreno_gpu *adreno_gpu,
+		const char *fwname);
 int adreno_hw_init(struct msm_gpu *gpu);
 uint32_t adreno_last_fence(struct msm_gpu *gpu);
 void adreno_recover(struct msm_gpu *gpu);
Signature · 6 lines
2.13.6

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw

PATCH 3/3 drm/msm/adreno: deal with linux-firmware fw paths

In reply to Rob Clark

When firmware was added to linux-firmware, it was put in a qcom sub- directory, unlike what we'd been using before. For a300_pfp.fw and a300_pm4.fw symlinks were created, but we'd prefer not to have to do this in the future. So add support to look in both places when loading firmware.

---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 64 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 21 +++++++++++
 2 files changed, 81 insertions(+), 4 deletions(-)
drivers/gpu/drm/msm/adreno/adreno_gpu.c +60 -4
index 19e12380297e..dc0db660c617 100644
@@ -69,12 +69,68 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char *fwname)
 {
 	struct drm_device *drm = adreno_gpu->base.dev;
 	const struct firmware *fw = NULL;
+	char newname[strlen("qcom/") + strlen(fwname) + 1];
 	int ret;
 
-	ret = request_firmware(&fw, fwname, drm->dev);
-	if (ret) {
-		dev_err(drm->dev, "failed to load %s: %d\n", fwname, ret);
-		return ERR_PTR(ret);
+	sprintf(newname, "qcom/%s", fwname);
+
+	/*
+	 * Try first to load from qcom/$fwfile using a direct load (to avoid
+	 * a potential timeout waiting for usermode helper)
+	 */
+	if ((adreno_gpu->fwloc == FW_LOCATION_UNKNOWN) ||
+	    (adreno_gpu->fwloc == FW_LOCATION_NEW)) {
+
+		ret = request_firmware_direct(&fw, newname, drm->dev);
+		if (!ret) {
+			dev_info(drm->dev, "loaded %s from new location\n",
+				newname);
+			adreno_gpu->fwloc = FW_LOCATION_NEW;
+			return fw;
+		} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) {
+			dev_err(drm->dev, "failed to load %s: %d\n",
+				newname, ret);
+			return ERR_PTR(ret);
+		}
+	}
+
+	/*
+	 * Then try the legacy location without qcom/ prefix
+	 */
+	if ((adreno_gpu->fwloc == FW_LOCATION_UNKNOWN) ||
+	    (adreno_gpu->fwloc == FW_LOCATION_LEGACY)) {
+
+		ret = request_firmware_direct(&fw, fwname, drm->dev);
+		if (!ret) {
+			dev_info(drm->dev, "loaded %s from legacy location\n",
+				newname);
+			adreno_gpu->fwloc = FW_LOCATION_LEGACY;
+			return fw;
+		} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) {
+			dev_err(drm->dev, "failed to load %s: %d\n",
+				fwname, ret);
+			return ERR_PTR(ret);
+		}
+	}
+
+	/*
+	 * Finally fall back to request_firmware() for cases where the
+	 * usermode helper is needed (I think mainly android)
+	 */
+	if ((adreno_gpu->fwloc == FW_LOCATION_UNKNOWN) ||
+	    (adreno_gpu->fwloc == FW_LOCATION_HELPER)) {
+
+		ret = request_firmware(&fw, newname, drm->dev);
+		if (!ret) {
+			dev_info(drm->dev, "loaded %s with helper\n",
+				newname);
+			adreno_gpu->fwloc = FW_LOCATION_HELPER;
+			return fw;
+		} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) {
+			dev_err(drm->dev, "failed to load %s: %d\n",
+				newname, ret);
+			return ERR_PTR(ret);
+		}
 	}
 
 	return fw;
drivers/gpu/drm/msm/adreno/adreno_gpu.h +21 -0
index 3eda98af546e..b8c22658929d 100644
@@ -101,6 +101,27 @@ struct adreno_gpu {
 	/* interesting register offsets to dump: */
 	const unsigned int *registers;
 
+	/*
+	 * Are we loading fw from legacy path?  Prior to addition
+	 * of gpu firmware to linux-firmware, the fw files were
+	 * placed in toplevel firmware directory, following qcom's
+	 * android kernel.  But linux-firmware preferred they be
+	 * placed in a 'qcom' subdirectory.
+	 *
+	 * For backwards compatibility, we try first to load from
+	 * the new path, using request_firmware_direct() to avoid
+	 * any potential timeout waiting for usermode helper, then
+	 * fall back to the old path (with direct load).  And
+	 * finally fall back to request_firmware() with the new
+	 * path to allow the usermode helper.
+	 */
+	enum {
+		FW_LOCATION_UNKNOWN = 0,
+		FW_LOCATION_NEW,       /* /lib/firmware/qcom/$fwfile */
+		FW_LOCATION_LEGACY,    /* /lib/firmware/$fwfile */
+		FW_LOCATION_HELPER,
+	} fwloc;
+
 	/* firmware: */
 	const struct firmware *pm4, *pfp;
 
Signature · 6 lines
2.13.6

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 20171016152749.23104-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#4 lore raw
Quoted 25 lines · Rob Clark

Previously, in an effort to defer initializing the gpu until firmware was available (ie. rootfs mounted), the gpu was not loaded at when the subdevice was bound. Which resulted that clks/etc were requested in a place that devm couldn't really help unwind if something failed.

Instead move request_firmware() to gpu->hw_init() and construct the gpu earlier in adreno_bind(). To avoid the rest of the driver needing to be aware of a gpu that hasn't managed to load firmware and hw_init() yet, stash the gpu ptr in the adreno device's drvdata, and don't set priv->gpu() until hw_init() succeeds.

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  6 +--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 78 +++++++++++++++---------------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 45 +++++++++++------
 drivers/gpu/drm/msm/msm_drv.c              | 10 ----
 4 files changed, 74 insertions(+), 65 deletions(-)
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +0 -0
index 17c59d839e6f..8df86d83d859 100644
@@ -536,13 +536,13 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
 		REG_A5XX_RBBM_SECVID_TSB_TRUSTED_BASE_HI, 0x00000000);

drivers/gpu/drm/msm/adreno/a5xx_gpu.c static int a5xx_hw_init(struct msm_gpu *gpu) · line 543

Quoted 10 lines
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +1 -3
 	gpu_write(gpu, REG_A5XX_RBBM_SECVID_TSB_TRUSTED_SIZE, 0x00000000);
 
-	/* Load the GPMU firmware before starting the HW init */
-	a5xx_gpmu_ucode_init(gpu);
-
 	ret = adreno_hw_init(gpu);
 	if (ret)
 		return ret;
 
+	/* Load the GPMU firmware before starting the HW init */

You can delete this comment since it is no longer accurate.

drivers/gpu/drm/msm/adreno/a5xx_gpu.c static int a5xx_hw_init(struct msm_gpu *gpu) · line 548

Quoted 5 lines
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +2 -0
+	a5xx_gpmu_ucode_init(gpu);
+
 	ret = a5xx_ucode_init(gpu);
 	if (ret)
 		return ret;

Jordan

Signature · 6 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
author
#5 lore raw
Quoted 35 lines · Jordan Crouse
Quoted 34 lines · Rob Clark

Previously, in an effort to defer initializing the gpu until firmware was available (ie. rootfs mounted), the gpu was not loaded at when the subdevice was bound. Which resulted that clks/etc were requested in a place that devm couldn't really help unwind if something failed.

Instead move request_firmware() to gpu->hw_init() and construct the gpu earlier in adreno_bind(). To avoid the rest of the driver needing to be aware of a gpu that hasn't managed to load firmware and hw_init() yet, stash the gpu ptr in the adreno device's drvdata, and don't set priv->gpu() until hw_init() succeeds.

---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  6 +--
 drivers/gpu/drm/msm/adreno/adreno_device.c | 78 +++++++++++++++---------------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 45 +++++++++++------
 drivers/gpu/drm/msm/msm_drv.c              | 10 ----
 4 files changed, 74 insertions(+), 65 deletions(-)
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +0 -3
index 17c59d839e6f..8df86d83d859 100644
@@ -536,13 +536,13 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
              REG_A5XX_RBBM_SECVID_TSB_TRUSTED_BASE_HI, 0x00000000);
      gpu_write(gpu, REG_A5XX_RBBM_SECVID_TSB_TRUSTED_SIZE, 0x00000000);
 
-     /* Load the GPMU firmware before starting the HW init */
-     a5xx_gpmu_ucode_init(gpu);
-
      ret = adreno_hw_init(gpu);
      if (ret)
              return ret;
 
Quoted 4 lines

drivers/gpu/drm/msm/adreno/a5xx_gpu.c static int a5xx_hw_init(struct msm_gpu *gpu) · line 543

Quoted 1 line
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +1 -0
+     /* Load the GPMU firmware before starting the HW init */

You can delete this comment since it is no longer accurate.

thanks, got that locally in a fixup

BR,
-R

Quoted 11 lines

drivers/gpu/drm/msm/adreno/a5xx_gpu.c static int a5xx_hw_init(struct msm_gpu *gpu) · line 548

Quoted 5 lines
drivers/gpu/drm/msm/adreno/a5xx_gpu.c +2 -0
+     a5xx_gpmu_ucode_init(gpu);
+
      ret = a5xx_ucode_init(gpu);
      if (ret)
              return ret;

Jordan

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project