Message ID | 20240906094025.638173-1-steven.price@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] drm/panthor: Display FW version information | expand |
This looks good on a G310 based platform with older FW from Mali DDK and Panthor driver. Output looks as follows: [ 5.896994] panthor 13000000.mali: [drm] Firmware git sha: b40270ac2191466fde1901ca0fbe8953dbf1d85c On 9/6/24, 2:40 AM, "Steven Price" <steven.price@arm.com <mailto:steven.price@arm.com>> wrote: The version number output when loading the firmware is actually the interface version not the version of the firmware itself. Update the message to make this clearer.
+1 I like the interface ver. :) On 9/6/24 10:40 AM, Steven Price wrote: > The version number output when loading the firmware is actually the > interface version not the version of the firmware itself. Update the > message to make this clearer. > > However, the firmware binary has a git SHA embedded into it which can be > used to identify which firmware binary is being loaded. So output this > as a drm_info() so that it's obvious from a dmesg log which firmware > binary is being used. > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > Signed-off-by: Steven Price <steven.price@arm.com> > --- > v2: > * Fix indentation > * Also update the FW interface message to include "using interface" to > make it clear it's not the FW version > * Add Reviewed-bys > > drivers/gpu/drm/panthor/panthor_fw.c | 57 +++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c > index 857f3f11258a..aea5dd9a4969 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > @@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type { > > /** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */ > CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4, > + > + /** > + * @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how > + * the FW binary was built. > + */ > + CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6 > }; > > #define CSF_FW_BINARY_ENTRY_TYPE(ehdr) ((ehdr) & 0xff) > @@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr { > } data; > }; > > +struct panthor_fw_build_info_hdr { > + /** @meta_start: Offset of the build info data in the FW binary */ > + u32 meta_start; > + /** @meta_size: Size of the build info data in the FW binary */ > + u32 meta_size; > +}; > + > /** > * struct panthor_fw_binary_iter - Firmware binary iterator > * > @@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > return 0; > } > > +static int panthor_fw_read_build_info(struct panthor_device *ptdev, > + const struct firmware *fw, > + struct panthor_fw_binary_iter *iter, > + u32 ehdr) > +{ > + struct panthor_fw_build_info_hdr hdr; > + char header[9]; > + const char git_sha_header[sizeof(header)] = "git_sha: "; > + int ret; > + > + ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr)); > + if (ret) > + return ret; > + > + if (hdr.meta_start > fw->size || > + hdr.meta_start + hdr.meta_size > fw->size) { > + drm_err(&ptdev->base, "Firmware build info corrupt\n"); > + /* We don't need the build info, so continue */ > + return 0; > + } > + > + if (memcmp(git_sha_header, fw->data + hdr.meta_start, > + sizeof(git_sha_header))) { > + /* Not the expected header, this isn't metadata we understand */ > + return 0; > + } > + > + /* Check that the git SHA is NULL terminated as expected */ > + if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') { > + drm_warn(&ptdev->base, "Firmware's git sha is not NULL terminated\n"); > + /* Don't treat as fatal */ > + return 0; > + } > + > + drm_info(&ptdev->base, "Firmware git sha: %s\n", > + fw->data + hdr.meta_start + sizeof(git_sha_header)); > + > + return 0; > +} > + > static void > panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload) > { > @@ -672,6 +725,8 @@ static int panthor_fw_load_entry(struct panthor_device *ptdev, > switch (CSF_FW_BINARY_ENTRY_TYPE(ehdr)) { > case CSF_FW_BINARY_ENTRY_TYPE_IFACE: > return panthor_fw_load_section_entry(ptdev, fw, &eiter, ehdr); > + case CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: > + return panthor_fw_read_build_info(ptdev, fw, &eiter, ehdr); > > /* FIXME: handle those entry types? */ > case CSF_FW_BINARY_ENTRY_TYPE_CONFIG: > @@ -921,7 +976,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev) > return ret; > } > > - drm_info(&ptdev->base, "CSF FW v%d.%d.%d, Features %#x Instrumentation features %#x", > + drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x", > CSF_IFACE_VERSION_MAJOR(glb_iface->control->version), > CSF_IFACE_VERSION_MINOR(glb_iface->control->version), > CSF_IFACE_VERSION_PATCH(glb_iface->control->version),
Hi Am 06.09.24 um 11:40 schrieb Steven Price: > The version number output when loading the firmware is actually the > interface version not the version of the firmware itself. Update the > message to make this clearer. > > However, the firmware binary has a git SHA embedded into it which can be > used to identify which firmware binary is being loaded. So output this > as a drm_info() so that it's obvious from a dmesg log which firmware > binary is being used. > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > Signed-off-by: Steven Price <steven.price@arm.com> > --- > v2: > * Fix indentation > * Also update the FW interface message to include "using interface" to > make it clear it's not the FW version > * Add Reviewed-bys > > drivers/gpu/drm/panthor/panthor_fw.c | 57 +++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c > index 857f3f11258a..aea5dd9a4969 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > @@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type { > > /** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */ > CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4, > + > + /** > + * @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how > + * the FW binary was built. > + */ > + CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6 > }; > > #define CSF_FW_BINARY_ENTRY_TYPE(ehdr) ((ehdr) & 0xff) > @@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr { > } data; > }; > > +struct panthor_fw_build_info_hdr { > + /** @meta_start: Offset of the build info data in the FW binary */ > + u32 meta_start; > + /** @meta_size: Size of the build info data in the FW binary */ > + u32 meta_size; > +}; > + > /** > * struct panthor_fw_binary_iter - Firmware binary iterator > * > @@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > return 0; > } > > +static int panthor_fw_read_build_info(struct panthor_device *ptdev, > + const struct firmware *fw, > + struct panthor_fw_binary_iter *iter, > + u32 ehdr) > +{ > + struct panthor_fw_build_info_hdr hdr; > + char header[9]; > + const char git_sha_header[sizeof(header)] = "git_sha: "; > + int ret; > + > + ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr)); > + if (ret) > + return ret; > + > + if (hdr.meta_start > fw->size || > + hdr.meta_start + hdr.meta_size > fw->size) { > + drm_err(&ptdev->base, "Firmware build info corrupt\n"); > + /* We don't need the build info, so continue */ > + return 0; > + } > + > + if (memcmp(git_sha_header, fw->data + hdr.meta_start, > + sizeof(git_sha_header))) { > + /* Not the expected header, this isn't metadata we understand */ > + return 0; > + } > + > + /* Check that the git SHA is NULL terminated as expected */ > + if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') { > + drm_warn(&ptdev->base, "Firmware's git sha is not NULL terminated\n"); > + /* Don't treat as fatal */ > + return 0; > + } > + > + drm_info(&ptdev->base, "Firmware git sha: %s\n", > + fw->data + hdr.meta_start + sizeof(git_sha_header)); Please consider making this debugging-only information. Printing takes time and the information is not essential unless for debugging. > + > + return 0; > +} > + > static void > panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload) > { > @@ -672,6 +725,8 @@ static int panthor_fw_load_entry(struct panthor_device *ptdev, > switch (CSF_FW_BINARY_ENTRY_TYPE(ehdr)) { > case CSF_FW_BINARY_ENTRY_TYPE_IFACE: > return panthor_fw_load_section_entry(ptdev, fw, &eiter, ehdr); > + case CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: > + return panthor_fw_read_build_info(ptdev, fw, &eiter, ehdr); > > /* FIXME: handle those entry types? */ > case CSF_FW_BINARY_ENTRY_TYPE_CONFIG: > @@ -921,7 +976,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev) > return ret; > } > > - drm_info(&ptdev->base, "CSF FW v%d.%d.%d, Features %#x Instrumentation features %#x", > + drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x", > CSF_IFACE_VERSION_MAJOR(glb_iface->control->version), > CSF_IFACE_VERSION_MINOR(glb_iface->control->version), > CSF_IFACE_VERSION_PATCH(glb_iface->control->version), Same. Best regards Thomas
Hi Thomas, On Mon, 9 Sep 2024 16:14:32 +0200 Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 06.09.24 um 11:40 schrieb Steven Price: > > The version number output when loading the firmware is actually the > > interface version not the version of the firmware itself. Update the > > message to make this clearer. > > > > However, the firmware binary has a git SHA embedded into it which can be > > used to identify which firmware binary is being loaded. So output this > > as a drm_info() so that it's obvious from a dmesg log which firmware > > binary is being used. > > > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > > Signed-off-by: Steven Price <steven.price@arm.com> > > --- > > v2: > > * Fix indentation > > * Also update the FW interface message to include "using interface" to > > make it clear it's not the FW version > > * Add Reviewed-bys > > > > drivers/gpu/drm/panthor/panthor_fw.c | 57 +++++++++++++++++++++++++++- > > 1 file changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c > > index 857f3f11258a..aea5dd9a4969 100644 > > --- a/drivers/gpu/drm/panthor/panthor_fw.c > > +++ b/drivers/gpu/drm/panthor/panthor_fw.c > > @@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type { > > > > /** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */ > > CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4, > > + > > + /** > > + * @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how > > + * the FW binary was built. > > + */ > > + CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6 > > }; > > > > #define CSF_FW_BINARY_ENTRY_TYPE(ehdr) ((ehdr) & 0xff) > > @@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr { > > } data; > > }; > > > > +struct panthor_fw_build_info_hdr { > > + /** @meta_start: Offset of the build info data in the FW binary */ > > + u32 meta_start; > > + /** @meta_size: Size of the build info data in the FW binary */ > > + u32 meta_size; > > +}; > > + > > /** > > * struct panthor_fw_binary_iter - Firmware binary iterator > > * > > @@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, > > return 0; > > } > > > > +static int panthor_fw_read_build_info(struct panthor_device *ptdev, > > + const struct firmware *fw, > > + struct panthor_fw_binary_iter *iter, > > + u32 ehdr) > > +{ > > + struct panthor_fw_build_info_hdr hdr; > > + char header[9]; > > + const char git_sha_header[sizeof(header)] = "git_sha: "; > > + int ret; > > + > > + ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr)); > > + if (ret) > > + return ret; > > + > > + if (hdr.meta_start > fw->size || > > + hdr.meta_start + hdr.meta_size > fw->size) { > > + drm_err(&ptdev->base, "Firmware build info corrupt\n"); > > + /* We don't need the build info, so continue */ > > + return 0; > > + } > > + > > + if (memcmp(git_sha_header, fw->data + hdr.meta_start, > > + sizeof(git_sha_header))) { > > + /* Not the expected header, this isn't metadata we understand */ > > + return 0; > > + } > > + > > + /* Check that the git SHA is NULL terminated as expected */ > > + if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') { > > + drm_warn(&ptdev->base, "Firmware's git sha is not NULL terminated\n"); > > + /* Don't treat as fatal */ > > + return 0; > > + } > > + > > + drm_info(&ptdev->base, "Firmware git sha: %s\n", > > + fw->data + hdr.meta_start + sizeof(git_sha_header)); > > Please consider making this debugging-only information. Printing takes > time and the information is not essential unless for debugging. Sounds like someone working on boot time optimization :-). More seriously, I don't mind downgrading those to debug messages, as long as we have the same information exposed through sysfs or DEV_QUERY, but I'd prefer doing that in a follow-up patch that takes care of all drm_info()s we have in panthor rather than addressing the two messages we're modifying in this patch. Regards, Boris
On Fri, 6 Sep 2024 10:40:25 +0100 Steven Price <steven.price@arm.com> wrote: > The version number output when loading the firmware is actually the > interface version not the version of the firmware itself. Update the > message to make this clearer. > > However, the firmware binary has a git SHA embedded into it which can be > used to identify which firmware binary is being loaded. So output this > as a drm_info() so that it's obvious from a dmesg log which firmware > binary is being used. > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > Signed-off-by: Steven Price <steven.price@arm.com> Queued to drm-misc-next. Thanks! Boris
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c index 857f3f11258a..aea5dd9a4969 100644 --- a/drivers/gpu/drm/panthor/panthor_fw.c +++ b/drivers/gpu/drm/panthor/panthor_fw.c @@ -78,6 +78,12 @@ enum panthor_fw_binary_entry_type { /** @CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA: Timeline metadata interface. */ CSF_FW_BINARY_ENTRY_TYPE_TIMELINE_METADATA = 4, + + /** + * @CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: Metadata about how + * the FW binary was built. + */ + CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA = 6 }; #define CSF_FW_BINARY_ENTRY_TYPE(ehdr) ((ehdr) & 0xff) @@ -132,6 +138,13 @@ struct panthor_fw_binary_section_entry_hdr { } data; }; +struct panthor_fw_build_info_hdr { + /** @meta_start: Offset of the build info data in the FW binary */ + u32 meta_start; + /** @meta_size: Size of the build info data in the FW binary */ + u32 meta_size; +}; + /** * struct panthor_fw_binary_iter - Firmware binary iterator * @@ -628,6 +641,46 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, return 0; } +static int panthor_fw_read_build_info(struct panthor_device *ptdev, + const struct firmware *fw, + struct panthor_fw_binary_iter *iter, + u32 ehdr) +{ + struct panthor_fw_build_info_hdr hdr; + char header[9]; + const char git_sha_header[sizeof(header)] = "git_sha: "; + int ret; + + ret = panthor_fw_binary_iter_read(ptdev, iter, &hdr, sizeof(hdr)); + if (ret) + return ret; + + if (hdr.meta_start > fw->size || + hdr.meta_start + hdr.meta_size > fw->size) { + drm_err(&ptdev->base, "Firmware build info corrupt\n"); + /* We don't need the build info, so continue */ + return 0; + } + + if (memcmp(git_sha_header, fw->data + hdr.meta_start, + sizeof(git_sha_header))) { + /* Not the expected header, this isn't metadata we understand */ + return 0; + } + + /* Check that the git SHA is NULL terminated as expected */ + if (fw->data[hdr.meta_start + hdr.meta_size - 1] != '\0') { + drm_warn(&ptdev->base, "Firmware's git sha is not NULL terminated\n"); + /* Don't treat as fatal */ + return 0; + } + + drm_info(&ptdev->base, "Firmware git sha: %s\n", + fw->data + hdr.meta_start + sizeof(git_sha_header)); + + return 0; +} + static void panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload) { @@ -672,6 +725,8 @@ static int panthor_fw_load_entry(struct panthor_device *ptdev, switch (CSF_FW_BINARY_ENTRY_TYPE(ehdr)) { case CSF_FW_BINARY_ENTRY_TYPE_IFACE: return panthor_fw_load_section_entry(ptdev, fw, &eiter, ehdr); + case CSF_FW_BINARY_ENTRY_TYPE_BUILD_INFO_METADATA: + return panthor_fw_read_build_info(ptdev, fw, &eiter, ehdr); /* FIXME: handle those entry types? */ case CSF_FW_BINARY_ENTRY_TYPE_CONFIG: @@ -921,7 +976,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev) return ret; } - drm_info(&ptdev->base, "CSF FW v%d.%d.%d, Features %#x Instrumentation features %#x", + drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x", CSF_IFACE_VERSION_MAJOR(glb_iface->control->version), CSF_IFACE_VERSION_MINOR(glb_iface->control->version), CSF_IFACE_VERSION_PATCH(glb_iface->control->version),