drm/msm: use firmware-name to find zap fw
Subsystems (7)
cover drm/msm: use firmware-name to find zap fw
From: Rob Clark <[email protected]>
For devices which use zap fw to take the GPU out of secure mode on reset, the firmware is likely to be signed with a device specific key. Meaning that we can't have a single filesystem (or /lib/firmware) that works on multiple devices.
So allow a firmware-name to be specified in the zap-shader node in dt. This moves the zap-shader node out of the core sdm845.dtsi and into per- device dts files. Which also removes the need for /delete-node/ in sdm845-cheza.dtsi (as cheza devices do not use zap).
This aligns with how Bjorn has been handling the similar situation with adsp/cdsp/mpss fw:
https://patchwork.kernel.org/patch/11160089/
Rob Clark (3):
drm/msm: support firmware-name for zap fw
dt-bindings: drm/msm/gpu: Document firmware-name
arm64: dts: sdm845: move gpu zap nodes to per-device dts
.../devicetree/bindings/display/msm/gpu.txt | 3 ++ arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 - arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 7 ++++ arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 8 +++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 +--- .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 7 ++++ drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 +++++++++++++++++-- 7 files changed, 55 insertions(+), 9 deletions(-)
Signature · 1 line
2.24.1
PATCH 1/3 drm/msm: support firmware-name for zap fw
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)
drivers/gpu/drm/msm/adreno/adreno_gpu.c +29 -3
@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;
@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +* +* If the firmware-name property is not found, for backwards +* compatibility we fall back to the fwname from the gpulist +* table. +*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname; + ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret); + fw =ERR_PTR (ret); + } + }else { +/* Request the MDT file for the firmware */ + fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +if (IS_ERR (fw)) {DRM_DEV_ERROR (dev,"Unable to load %s\n" , fwname);return PTR_ERR (fw);
@@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,* not. But since we've already gotten through adreno_request_fw() * we know which of the two cases it is: */ -if (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY ) { +if (signed_fwname || (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY )) { ret =qcom_mdt_load (dev, fw, fwname, pasid, mem_region, mem_phys, mem_size,NULL ); }else {
Signature · 1 line
2.24.1
Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw
Quoted 40 lines · Rob Clark
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/adreno/adreno_gpu.c +12
-2@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +* +* If the firmware-name property is not found, for backwards
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 82
Quoted 10 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +10
-0+* compatibility we fall back to the fwname from the gpulist +* table. +*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname; + ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret); + fw =ERR_PTR (ret);
Perhaps just return ret; here, or omit the error print here any rely solely on the one below?
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 85
Quoted 3 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +3
-0+ } + }else { +/* Request the MDT file for the firmware */
How about changing this to "Request the MDT file from the default location" or something like that?
Rest looks good.
Regards,
Bjorn
Quoted 18 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +4
-1+ fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +if (IS_ERR (fw)) {DRM_DEV_ERROR (dev,"Unable to load %s\n" , fwname);return PTR_ERR (fw);@@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,* not. But since we've already gotten through adreno_request_fw() * we know which of the two cases it is: */ -if (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY ) { +if (signed_fwname || (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY )) { ret =qcom_mdt_load (dev, fw, fwname, pasid, mem_region, mem_phys, mem_size,NULL ); }else {Signature · 1 line
2.24.1
Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw
In reply to Rob ClarkQuoted 39 lines · Rob Clark
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/adreno/adreno_gpu.c +11
-2@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +*
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 81
Quoted 10 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +10
-0+* If the firmware-name property is not found, for backwards +* compatibility we fall back to the fwname from the gpulist +* table. +*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname; + ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret);
Could adreno_request_fw be called with fwname if request_firmware_direct fails ?
Quoted 19 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +8
-1+ fw =ERR_PTR (ret); + } + }else { +/* Request the MDT file for the firmware */ + fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +if (IS_ERR (fw)) {DRM_DEV_ERROR (dev,"Unable to load %s\n" , fwname);return PTR_ERR (fw);@@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,* not. But since we've already gotten through adreno_request_fw() * we know which of the two cases it is: */ -if (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY ) { +if (signed_fwname || (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY )) { ret =qcom_mdt_load (dev, fw, fwname, pasid, mem_region, mem_phys, mem_size,NULL ); }else {
Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw
Quoted 45 lines · Tom Rix
Quoted 42 lines · Rob Clark
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/adreno/adreno_gpu.c +14
-2@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +* +* If the firmware-name property is not found, for backwards +* compatibility we fall back to the fwname from the gpulist +* table.
Quoted 8 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 81
Quoted 7 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +7
-0+*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname; + ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret);Could
adreno_request_fwbe called with fwname ifrequest_firmware_directfails ?
*possibly*.. initially I avoided this because the failure mode for incorrectly signed firmware was silent and catestrophic. But Bjorn tells me this has been fixed.. in which case we could try and detect if it is the incorrect fw. I need to try some experiments to confirm we can detect this case properly.
BR,
-R
Quoted 20 lines
Quoted 19 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +8
-1+ fw =ERR_PTR (ret); + } + }else { +/* Request the MDT file for the firmware */ + fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +if (IS_ERR (fw)) {DRM_DEV_ERROR (dev,"Unable to load %s\n" , fwname);return PTR_ERR (fw);@@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,* not. But since we've already gotten through adreno_request_fw() * we know which of the two cases it is: */ -if (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY ) { +if (signed_fwname || (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY )) { ret =qcom_mdt_load (dev, fw, fwname, pasid, mem_region, mem_phys, mem_size,NULL ); }else {
Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw
Quoted 57 lines · Rob Clark
Quoted 45 lines · Tom Rix
Quoted 42 lines · Rob Clark
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/adreno/adreno_gpu.c +14
-2@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +* +* If the firmware-name property is not found, for backwards +* compatibility we fall back to the fwname from the gpulist +* table. Quoted 8 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 81
Quoted 7 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +7
-0+*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname; + ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret);Could
adreno_request_fwbe called with fwname ifrequest_firmware_directfails ?
Quoted 5 lines
*possibly*.. initially I avoided this because the failure mode for incorrectly signed firmware was silent and catestrophic. But Bjorn tells me this has been fixed.. in which case we could try and detect if it is the incorrect fw. I need to try some experiments to confirm we can detect this case properly.
I've been thinking about fallback to using fwname from the gpulist table, but I think I don't really like the idea.
The failure mode for not falling back to $firmware/qcom/$fwname is pretty safe.. you simply don't get gpu accel, but display and everything else works, so you can boot up far enough to see what the problem is and fix it. But the failure mode if, with some device's scm fw doesn't report incorrectly signed zap fw, is pretty catastrophic (insta-reboot, which can be hard to debug on phone/laptop without debug uart).
Since we haven't pushed the test-key signed qcom/sdm835/a630_zap.mbn to linux-firmware yet, I think this is a good time to make this switch for a6xx.
For a530 devices, maybe we hold of adding firmware-name to dt until linux-firmware gains a qcom/msm8996/a530_zap.mbn (which would be a good time to switch to the packed .mbn instead of split .mdt + .b0* files).. or just not specify a firmware-name for device using test-key signed zap and fallback to adreno_request_fw() and the existing path.
Fortunately zap is not used prior to a5xx, so we mainly just need to care about backwards compat for a530.
BR,
-R
Quoted 23 lines
BR,
-RQuoted 20 lines
Quoted 19 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +8
-1+ fw =ERR_PTR (ret); + } + }else { +/* Request the MDT file for the firmware */ + fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +if (IS_ERR (fw)) {DRM_DEV_ERROR (dev,"Unable to load %s\n" , fwname);return PTR_ERR (fw);@@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,* not. But since we've already gotten through adreno_request_fw() * we know which of the two cases it is: */ -if (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY ) { +if (signed_fwname || (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY )) { ret =qcom_mdt_load (dev, fw, fwname, pasid, mem_region, mem_phys, mem_size,NULL ); }else {
Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw
In reply to Rob ClarkQuoted 46 lines · Rob Clark
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/adreno/adreno_gpu.c +18
-2@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +* +* If the firmware-name property is not found, for backwards +* compatibility we fall back to the fwname from the gpulist +* table. +*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname;
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 88
Quoted 10 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +10
-0+ ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret); + fw =ERR_PTR (ret); + } + }else { +/* Request the MDT file for the firmware */ + fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +
Since DT seems to be the trend for target specific firmware names I think we should plan to quickly deprecate the legacy name and not require new targets to set it. If a zap node is going to be opt in then it isn't onerous to ask the developer to set the additional property for each target platform.
Jordan
Quoted 15 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +1
-1if (IS_ERR (fw)) {DRM_DEV_ERROR (dev,"Unable to load %s\n" , fwname);return PTR_ERR (fw);@@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,* not. But since we've already gotten through adreno_request_fw() * we know which of the two cases it is: */ -if (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY ) { +if (signed_fwname || (to_adreno_gpu (gpu)->fwloc ==FW_LOCATION_LEGACY )) { ret =qcom_mdt_load (dev, fw, fwname, pasid, mem_region, mem_phys, mem_size,NULL ); }else {Signature · 1 line
2.24.1
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw
Quoted 54 lines · Jordan Crouse
Quoted 53 lines · Rob Clark
From: Rob Clark <[email protected]>
Since zap firmware can be device specific, allow for a firmware-name property in the zap node to specify which firmware to load, similarly to the scheme used for dsp/wifi/etc.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)drivers/gpu/drm/msm/adreno/adreno_gpu.c +25
-2@@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, {struct device *dev = &gpu->pdev->dev;const struct firmware *fw; +const char *signed_fwname =NULL ;struct device_node *np, *mem_np;struct resource r;phys_addr_t mem_phys;@@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, mem_phys = r.start; -/* Request the MDT file for the firmware */ - fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); +/* +* Check for a firmware-name property. This is the new scheme +* to handle firmware that may be signed with device specific +* keys, allowing us to have a different zap fw path for different +* devices. +* +* If the firmware-name property is found, we bypass the +* adreno_request_fw() mechanism, because we don't need to handle +* the /lib/firmware/qcom/* vs /lib/firmware/* case. +* +* If the firmware-name property is not found, for backwards +* compatibility we fall back to the fwname from the gpulist +* table. +*/ +of_property_read_string_index (np,"firmware-name" ,0 , &signed_fwname); +if (signed_fwname) { + fwname = signed_fwname; + ret =request_firmware_direct (&fw, signed_fwname, gpu->dev->dev); +if (ret) { +DRM_DEV_ERROR (dev,"could not load signed zap firmware: %d\n" , ret); + fw =ERR_PTR (ret); + } + }else { +/* Request the MDT file for the firmware */
Quoted 9 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname, · line 88
Quoted 3 lines
drivers/gpu/drm/msm/adreno/adreno_gpu.c +3
-0+ fw =adreno_request_fw (to_adreno_gpu (gpu), fwname); + } +Since DT seems to be the trend for target specific firmware names I think we should plan to quickly deprecate the legacy name and not require new targets to set it. If a zap node is going to be opt in then it isn't onerous to ask the developer to set the additional property for each target platform.
For the zap specifically I agree that it would be nice to require this property, but for non-zap firmware it seems reasonable to continue with the existing scheme.
Regards,
Bjorn
PATCH 2/3 dt-bindings: drm/msm/gpu: Document firmware-name
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
The firmware-name property in the zap node can be used to specify a device specific zap firmware.
--- Documentation/devicetree/bindings/display/msm/gpu.txt | 3 +++ 1 file changed, 3 insertions(+)
Documentation/devicetree/bindings/display/msm/gpu.txt +3 -0
@@ -33,6 +33,8 @@ Required properties: - zap-shader: For a5xx and a6xx devices this node contains a memory-region that points to reserved memory to store the zap shader that can be used to help bring the GPU out of secure mode. +- firmware-name: optional property of the 'zap-shader' node, listing the + relative path of the device specific zap firmware. Example 3xx/4xx/a5xx:
@@ -85,6 +87,7 @@ Example a6xx (with GMU): zap-shader { memory-region = <&zap_shader_region>; + firmware-name = "qcom/LENOVO/81JL/qcdxkmsuc850.mbn" }; }; };
Signature · 1 line
2.24.1
Re: [PATCH 2/3] dt-bindings: drm/msm/gpu: Document firmware-name
Quoted 6 lines · Rob Clark
From: Rob Clark <[email protected]>
The firmware-name property in the zap node can be used to specify a device specific zap firmware.
Signed-off-by Rob Clark <[email protected]>
Quoted 28 lines
--- Documentation/devicetree/bindings/display/msm/gpu.txt | 3 +++ 1 file changed, 3 insertions(+)Documentation/devicetree/bindings/display/msm/gpu.txt +3
-0@@ -33,6 +33,8 @@ Required properties: - zap-shader: For a5xx and a6xx devices this node contains a memory-region that points to reserved memory to store the zap shader that can be used to help bring the GPU out of secure mode. +- firmware-name: optional property of the 'zap-shader' node, listing the + relative path of the device specific zap firmware. Example 3xx/4xx/a5xx:@@ -85,6 +87,7 @@ Example a6xx (with GMU): zap-shader { memory-region = <&zap_shader_region>; + firmware-name = "qcom/LENOVO/81JL/qcdxkmsuc850.mbn" }; }; };Signature · 1 line
2.24.1
PATCH 3/3 arm64: dts: sdm845: move gpu zap nodes to per-device dts
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
We want to specify per-device firmware-name, so move the zap node into the .dts file for individual boards/devices. This lets us get rid of the /delete-node/ for cheza, which does not use zap.
--- arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 - arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 7 +++++++ arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 8 ++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 +----- arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 7 +++++++ 5 files changed, 23 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi +0 -1
@@ -165,7 +165,6 @@ panel_in_edp: endpoint { /delete-node/ &venus_mem; /delete-node/ &cdsp_mem; /delete-node/ &cdsp_pas; -/delete-node/ &zap_shader; /delete-node/ &gpu_mem; /* Increase the size from 120 MB to 128 MB */
arch/arm64/boot/dts/qcom/sdm845-db845c.dts +7 -0
@@ -352,6 +352,13 @@ &gcc { <GCC_QSPI_CNOC_PERIPH_AHB_CLK>; }; +&gpu { + zap-shader { + memory-region = <&gpu_mem>; + firmware-name = "qcom/db845c/a630_zap.mbn"; + }; +}; + &pm8998_gpio { vol_up_pin_a: vol-up-active { pins = "gpio6";
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +8 -0
@@ -360,6 +360,14 @@ &gcc { <GCC_LPASS_SWAY_CLK>; }; +&gpu { + zap-shader { + memory-region = <&gpu_mem>; + // TODO presumably mtp can use same "test key" signed zap? + firmware-name = "qcom/db845c/a630_zap.mbn"; + }; +}; + &i2c10 { status = "okay"; clock-frequency = <400000>;
arch/arm64/boot/dts/qcom/sdm845.dtsi +1 -5
@@ -2804,7 +2804,7 @@ dsi1_phy: dsi-phy@ae96400 { }; }; - gpu@5000000 { + gpu: gpu@5000000 { compatible = "qcom,adreno-630.2", "qcom,adreno"; #stream-id-cells = <16>;
@@ -2824,10 +2824,6 @@ gpu@5000000 { qcom,gmu = <&gmu>; - zap_shader: zap-shader { - memory-region = <&gpu_mem>; - }; - gpu_opp_table: opp-table { compatible = "operating-points-v2";
arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +7 -0
@@ -245,6 +245,13 @@ &gcc { <GCC_QSPI_CNOC_PERIPH_AHB_CLK>; }; +&gpu { + zap-shader { + memory-region = <&gpu_mem>; + firmware-name = "qcom/LENOVO/81JL/qcdxkmsuc850.mbn"; + }; +}; + &i2c1 { status = "okay"; clock-frequency = <400000>;
Signature · 1 line
2.24.1
Re: [PATCH 3/3] arm64: dts: sdm845: move gpu zap nodes to per-device dts
Quoted 32 lines · Rob Clark
From: Rob Clark <[email protected]>
We want to specify per-device firmware-name, so move the zap node into the .dts file for individual boards/devices. This lets us get rid of the /delete-node/ for cheza, which does not use zap.
Signed-off-by Rob Clark <[email protected]>--- arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 - arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 7 +++++++ arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 8 ++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 +----- arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 7 +++++++ 5 files changed, 23 insertions(+), 6 deletions(-)arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi +0
-1@@ -165,7 +165,6 @@ panel_in_edp: endpoint { /delete-node/ &venus_mem; /delete-node/ &cdsp_mem; /delete-node/ &cdsp_pas; -/delete-node/ &zap_shader; /delete-node/ &gpu_mem; /* Increase the size from 120 MB to 128 MB */arch/arm64/boot/dts/qcom/sdm845-db845c.dts +0
-0
arch/arm64/boot/dts/qcom/sdm845-db845c.dts &gcc { · line 358
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-db845c.dts +4
-0@@ -352,6 +352,13 @@ &gcc { <GCC_QSPI_CNOC_PERIPH_AHB_CLK>; }; +&gpu { + zap-shader { + memory-region = <&gpu_mem>; + firmware-name = "qcom/db845c/a630_zap.mbn";
We agreed upon qcom/sdm845/* for the test-signed 845 firmware :)
Quoted 11 lines
arch/arm64/boot/dts/qcom/sdm845-db845c.dts +3
-0+ }; +}; + &pm8998_gpio { vol_up_pin_a: vol-up-active { pins = "gpio6";arch/arm64/boot/dts/qcom/sdm845-mtp.dts +0
-0
arch/arm64/boot/dts/qcom/sdm845-mtp.dts &gcc { · line 366
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +4
-0@@ -360,6 +360,14 @@ &gcc { <GCC_LPASS_SWAY_CLK>; }; +&gpu { + zap-shader { + memory-region = <&gpu_mem>; + // TODO presumably mtp can use same "test key" signed zap?
Drop this comment after s/db845c/sdm845/
arch/arm64/boot/dts/qcom/sdm845-mtp.dts &gcc { · line 367
Quoted 1 line
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +1
-0+ firmware-name = "qcom/db845c/a630_zap.mbn";
Apart from that, I think this looks good!
Regards,
Bjorn
Quoted 51 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +3
-0+ }; +}; + &i2c10 { status = "okay"; clock-frequency = <400000>;arch/arm64/boot/dts/qcom/sdm845.dtsi +1
-5@@ -2804,7 +2804,7 @@ dsi1_phy: dsi-phy@ae96400 { }; }; - gpu@5000000 { + gpu: gpu@5000000 { compatible = "qcom,adreno-630.2", "qcom,adreno"; #stream-id-cells = <16>;@@ -2824,10 +2824,6 @@ gpu@5000000 { qcom,gmu = <&gmu>; - zap_shader: zap-shader { - memory-region = <&gpu_mem>; - }; - gpu_opp_table: opp-table { compatible = "operating-points-v2";arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts +7
-0@@ -245,6 +245,13 @@ &gcc { <GCC_QSPI_CNOC_PERIPH_AHB_CLK>; }; +&gpu { + zap-shader { + memory-region = <&gpu_mem>; + firmware-name = "qcom/LENOVO/81JL/qcdxkmsuc850.mbn"; + }; +}; + &i2c1 { status = "okay"; clock-frequency = <400000>;Signature · 1 line
2.24.1