[PATCH] drm/panthor: Display FW version information

[Carsten Haitzler]

On 9/5/24 10:08 PM, Liviu Dudau wrote:
> On Thu, Sep 05, 2024 at 06:45:28PM +0200, Boris Brezillon wrote:
>> On Thu,  5 Sep 2024 16:51:44 +0100
>> Steven Price <steven.price at arm.com> wrote:
>>
>>> The firmware binary has a git SHA embedded into it which can be used to
>>> identify which firmware binary is being loaded. Output this as a
>>> drm_info() so that it's obvious from a dmesg log which firmware binary
>>> is being used.
>>>
>>> Signed-off-by: Steven Price <steven.price at arm.com>
>>
>> Just one formatting issue mentioned below, looks good otherwise.
>>
>> Reviewed-by: Boris Brezillon <boris.brezillon at collabora.com>
> 
> Reviewed-by: Liviu Dudau <liviu.dudau at arm.com>
> 
>>
>>> ---
>>>   drivers/gpu/drm/panthor/panthor_fw.c | 55 ++++++++++++++++++++++++++++
>>>   1 file changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
>>> index 857f3f11258a..ef007287575c 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))) {
>>
>> Indentation seems broken here:
>>
>> 	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));
>>
>> Maybe we should also change the "FW vX.Y.Z" message into "FW interface
>> vX.Y.Z" to clarify things.
> 
> Or something like "FW using interface vX.Y.Z"

+1. I like the idea. It's a bit confusing otherwise.

> Best regards,
> Liviu
> 
>>
>>> +
>>> +	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:
>>
>