diff mbox series

[v2] drm/panthor: Display FW version information

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

Commit Message

Steven Price Sept. 6, 2024, 9:40 a.m. UTC 
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(-)

Comments

Healy, Christopher Sept. 6, 2024, 5:16 p.m. UTC | #1 
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.
Carsten Haitzler Sept. 9, 2024, 2:01 p.m. UTC | #2 
+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),
Thomas Zimmermann Sept. 9, 2024, 2:14 p.m. UTC | #3 
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
Boris Brezillon Sept. 9, 2024, 2:47 p.m. UTC | #4 
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
Boris Brezillon Sept. 12, 2024, 7:34 a.m. UTC | #5 
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 mbox series

Patch

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),